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

fix(magic-contracts): Add missing docstrings in magic contracts #3457

Merged
merged 8 commits into from
Jul 25, 2024

Conversation

Ginowine
Copy link
Contributor

@Ginowine Ginowine commented Jul 3, 2024

Description of change

Some of the magic contract reference docs are missing information about using the functions. This PR adds Solidity docstrings to the functions of the following contracts:

  • ISC
  • ISCAccounts
  • ISCPrivileged
  • ISCSandbox
  • ISCTypes
  • ISCUtil

Links to any relevant issues

`fixes issue #3422

Type of change

  • Documentation Fix

Change checklist

Tick the boxes that are relevant to your changes, and delete any items that are not.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have selected the develop branch as the target branch

@Ginowine Ginowine added the documentation Improvements or additions to documentation label Jul 3, 2024
Copy link
Contributor

@salaheldinsoliman salaheldinsoliman left a comment

Choose a reason for hiding this comment

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

@Ginowine LGTM! except for this small comment:
NatSpec tags are all optional. That is, if @notice explains what the function does, there is no need to add a @dev. only add it when there are extra details
Can you please give it another small iteration, where if a @notice suffices just remove @dev
Thanks

@Ginowine Ginowine marked this pull request as ready for review July 4, 2024 19:57
@Ginowine
Copy link
Contributor Author

Ginowine commented Jul 4, 2024

@Ginowine LGTM! except for this small comment: NatSpec tags are all optional. That is, if @notice explains what the function does, there is no need to add a @dev. only add it when there are extra details Can you please give it another small iteration, where if a @notice suffices just remove @dev Thanks

@salaheldinsoliman. Ok great, thanks. I'll go through it again.

@Ginowine Ginowine force-pushed the devx/update-docStrings-in-magic-contracts branch from 0346b35 to 12a1467 Compare July 9, 2024 08:50
@lucas-tortora
Copy link
Contributor

could you rename the PR to follow the guidelines on confluence?

@Ginowine Ginowine force-pushed the devx/update-docStrings-in-magic-contracts branch from a9188c4 to 98b38d3 Compare July 22, 2024 10:29
@Ginowine Ginowine changed the title devx/add missing docstrings in magic contracts fix(magic-contracts): Add missing docstrings in magic contracts Jul 22, 2024
packages/vm/core/evm/iscmagic/ISCAccounts.sol Outdated Show resolved Hide resolved
packages/vm/core/evm/iscmagic/ISCAccounts.sol Outdated Show resolved Hide resolved
packages/vm/core/evm/iscmagic/ISCSandbox.sol Outdated Show resolved Hide resolved
packages/vm/core/evm/iscmagic/ISCSandbox.sol Outdated Show resolved Hide resolved
packages/vm/core/evm/iscmagic/ISCSandbox.sol Outdated Show resolved Hide resolved
packages/vm/core/evm/iscmagic/ISCTypes.sol Show resolved Hide resolved
@jorgemmsilva jorgemmsilva enabled auto-merge July 24, 2024 14:33
@Ginowine Ginowine force-pushed the devx/update-docStrings-in-magic-contracts branch from 7d46a6c to 9307c85 Compare July 24, 2024 17:11
@jorgemmsilva jorgemmsilva merged commit cecf521 into develop Jul 25, 2024
5 checks passed
@jorgemmsilva jorgemmsilva deleted the devx/update-docStrings-in-magic-contracts branch July 25, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants