Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs: Improve Accessors and Manager contracts documentation #507

Merged
merged 6 commits into from
Feb 16, 2023

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Feb 15, 2023

This PR:

  1. Unifying the comment formatting
  2. Commenting on the uncommented
  3. Improving the existing documentation

@mmv08 mmv08 requested review from rmeissner and Uxio0 February 15, 2023 14:16
@mmv08 mmv08 force-pushed the docs/improve-natspec-docs branch from b49fd97 to becf156 Compare February 15, 2023 14:17
"prettier": "^2.8.3",
"prettier-plugin-solidity": "1.1.1",
"prettier": "^2.8.4",
"prettier-plugin-solidity": "1.1.2",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keeping the formatters up-to-date in case there are any improvements for docs n code formatting

@coveralls
Copy link

coveralls commented Feb 15, 2023

Pull Request Test Coverage Report for Build 4193190861

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 7 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 97.053%

Files with Coverage Reduction New Missed Lines %
contracts/SafeL2.sol 7 0%
Totals Coverage Status
Change from base Build 4112927572: 0.0%
Covered Lines: 307
Relevant Lines: 315

💛 - Coveralls

@mmv08 mmv08 force-pushed the docs/improve-natspec-docs branch 2 times, most recently from 8828a42 to 0e1a44f Compare February 15, 2023 14:20
/// @author Richard Meissner - <[email protected]>
/**
* @title Simulate Transaction Accessor.
* @notice Can be used with StorageAccessible to simulate Safe transactions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be @dev?
image

Copy link
Member Author

@mmv08 mmv08 Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had an internal debate on this one. I reasoned that the end user of this particular contract is a developer, so notice is the way to go. Also, the docs mention that @dev is for extra details and we have none

/// @param data Data payload of module transaction.
/// @param operation Operation type of module transaction.
/**
* @notice Execute `operation` (0: Call, 1: DelegateCall) to `to` with `data` and `value` (Native Token)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have the full data in here? might be quite long ... but not sure what a good alternative would be. At least I would exchange value and data in the text to avoid that the value gets lost after a long data.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed data

@mmv08 mmv08 force-pushed the docs/improve-natspec-docs branch from 0e1a44f to cc348ed Compare February 16, 2023 10:49
@mmv08 mmv08 merged commit 6b3784f into main Feb 16, 2023
@mmv08 mmv08 deleted the docs/improve-natspec-docs branch February 16, 2023 12:43
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants