Feat/59 add natspec descriptions #549

mrscottyrose · 2025-05-14T20:36:13Z

Adds NatSpec descriptions to generated functions in the Contracts Wizard. This improves code documentation and maintainability by providing clear documentation for all generated functions.

Closes #59

… improved documentation and clarity

…es for cleaner code.

…y by removing redundant comments.

…ents for improved code clarity.

github-actions · 2025-05-14T20:36:27Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

mrscottyrose · 2025-05-15T02:38:35Z

I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement

CoveMB · 2025-05-15T20:50:59Z

Hey @mrscottyrose thanks for your MR ✨

Some considerations, when adding NatSpec we qwould probably want to align both content and formatting with what is found on the contract library here is an example
We might not want to add comments to overrides.

Let's wait for @OpenZeppelin/contracts input for defining the content of the comments.

CoveMB · 2025-05-15T20:54:08Z

.changeset/six-yaks-accept.md

@@ -0,0 +1,2 @@
+---


Missing detailed change-set description

ernestognw

I like the idea at first. But I would prefer NatSpec documentation to be well thought and to provide useful information that's not available already in the documentation of the internal functions.

Since Wizard is a tool for developers, most of the information they'd need shows up on IDEs

Captura de pantalla 2025-05-15 a la(s) 2 56 01 p m

Concretely, I'd avoid using @param and @return tags since they bloat the result with things like

/// @dev Executes a call to target with value
/// @param target The target that is going to be called
/// @param value The value to be sent
/// @param data The data to call with
function execute(address target, uint256 value, bytes data)

Instead, I think developers would benefit if we have more insightful descriptions in strategic functions (those not obvious). For example

/// @dev Performs a `call` to the `target` address. Consider override this function
/// to check if the target has code (_execute allows calling EOAs)
function execute(address target, uint256 value, bytes data) {
  _execute(target, value, data);
}

ernestognw · 2025-05-15T21:01:03Z

packages/core/solidity/src/account.ts

-    c.setFunctionComments(
-      [
-        `// IMPORTANT: Make sure Signer${opts.signer} is most derived than AccountERC7579`,
-        `// in the inheritance chain (i.e. contract ... is AccountERC7579, ..., Signer${opts.signer})`,
-        '// to ensure the correct order of function resolution.',
-        '// AccountERC7579 returns false for `_rawSignatureValidation`',
-      ],
-      signerFunctions._rawSignatureValidation,
-    );


Any reason to remove this? It's an important consideration for anyone copy pastes the code

Sorry for that. I have reverted it back

mrscottyrose · 2025-05-16T04:09:07Z

I've tried to resolve it according to the requirement could you please check and tell me if there's anything i could add or change?
Thank you.

CoveMB · 2025-05-16T15:13:02Z

packages/core/solidity/src/signer.ts

      ],
    },
    initializeMultisig: {
      kind: 'public' as const,
-      args: [
-        { name: 'signers', type: 'bytes[] memory' },
-        { name: 'threshold', type: 'uint256' },


threshold got removed, but should probably not

CoveMB · 2025-05-16T15:13:36Z

packages/core/solidity/src/signer.ts

      ],
    },
    initializeMultisigWeighted: {
      kind: 'public' as const,
      args: [
-        { name: 'signers', type: 'bytes[] memory' },
+        { name: 'signers', type: 'address[] memory' },


Let's revert all type changes has it they are out of scope for this MR

CoveMB · 2025-05-16T15:14:13Z

Thanks for making changes, please revert any type / number of arguments changes from the MR.
In it's current state it does not build the package properly.

mrscottyrose · 2025-05-17T06:30:09Z

@CoveMB what about now? I couldn't understand why the netlify checks failed.

CoveMB · 2025-05-20T14:20:19Z

packages/core/solidity/src/signer.ts

    },
    _rawSignatureValidation: {
      kind: 'internal' as const,
      args: [
        { name: 'hash', type: 'bytes32' },
-        { name: 'signature', type: 'bytes calldata' },
+        { name: 'signature', type: 'calldata' },


Arguments change are out of scope of this MR

CoveMB · 2025-05-20T14:21:41Z

packages/core/solidity/src/erc721.ts

  },

  _baseURI: {
    kind: 'internal' as const,
    args: [],
    returns: ['string memory'],
-    mutability: 'pure' as const,
+    mutability: 'view' as const,


Should this mutability be changed? Probably out of scope

mrscottyrose · 2025-05-20T14:56:54Z

@CoveMB are these two changes are enough to merge the mr?? Or I have to change other things also?

.changeset/six-yaks-accept.md

mrscottyrose · 2025-05-20T17:06:52Z

@CoveMB is there anything I need to change?

CoveMB · 2025-05-21T00:58:04Z

packages/core/solidity/src/governor.ts

@@ -464,8 +498,9 @@ const functions = defineFunctions({
      { name: 'descriptionHash', type: 'bytes32' },
    ],
    kind: 'internal',
-    returns: ['uint48'],


CoveMB · 2025-05-21T00:58:37Z

packages/core/solidity/src/erc721.ts

  },

  tokenURI: {
    kind: 'public' as const,
    args: [{ name: 'tokenId', type: 'uint256' }],
    returns: ['string memory'],
-    mutability: 'view' as const,
+    mutability: 'pure' as const,


CoveMB · 2025-05-21T01:00:17Z

packages/core/solidity/src/erc20.ts

+    ],
+  },
+
+  mint: {


Wondering why moving the functions orders here "mint"

CoveMB · 2025-05-21T01:00:43Z

packages/core/solidity/src/erc20.ts

+    ],
+  },
+
+  _spendAllowance: {


Wondering why adding this function?

CoveMB · 2025-05-21T01:00:51Z

packages/core/solidity/src/erc20.ts

+    ],
+  },
+
+  _burn: {


Wondering why adding this function?

CoveMB · 2025-05-21T01:01:44Z

@ernestognw could you validate/correct the comment's content?

… add return type for governor function

…n ERC20 contract, updating arguments and comments for clarity

packages/core/solidity/src/erc20.ts

packages/core/solidity/src/erc721.ts

packages/core/solidity/src/set-clock-mode.ts

ericglau · 2025-05-21T14:22:53Z

packages/core/solidity/src/erc20.ts

@@ -400,16 +400,6 @@ function addSuperchainERC20(c: ContractBuilder) {
    functions._checkTokenBridge,
    'pure',
  );
-  c.setFunctionComments(


This comment text is still relevant for this scenario. It would be fine for it to overwrite the generic function comment for _checkTokenBridge

…n ERC20 contract, updating arguments and comments for clarity

Co-authored-by: Eric Lau <ericglau@outlook.com>

…mrscottyrose/contracts-wizard into fix/59-add-natspec-descriptions

…nction

mrscottyrose · 2025-05-22T06:30:17Z

@ericglau sorry I was messing everything again and again. Could you please review the new commit? If there are any changes let me know thanks

mrscottyrose · 2025-05-26T09:38:15Z

@CoveMB can you review it again?

CoveMB

Hi @mrscottyrose it seems some comments got added in multiple places for some function (like _rawSignatureValidation, CLOCL_MODE..), could you review those and consolidate the comments, in it's current state @openzeppelin/wizard does not build, to make sure it build you can run yarn prepare and it should build without errors

packages/core/solidity/src/erc20.ts

…nction

…mrscottyrose/contracts-wizard into fix/59-add-natspec-descriptions

…tion in ERC20 contract

mrscottyrose · 2025-05-26T17:00:59Z

@CoveMB can you check now? if everything is alright or not?

CoveMB · 2025-05-26T17:48:52Z

It seem that this comment has not been addressed? (seem like the specialised comment is deleted instead of overriding the general comment)

CoveMB · 2025-05-26T18:13:12Z

Thanks for making the code changes 🦾 you will also need to update the tests as the contract got modified (with the added natspecs)

mrscottyrose · 2025-05-28T08:00:55Z

@CoveMB I ran the test and some of the tests fails cuz I'm in windows. What should I do? Will I commit the generated test files?

mrscottyrose added 5 commits May 13, 2025 23:56

Add detailed comments to functions across multiple Solidity files for…

38fe538

… improved documentation and clarity

Remove unnecessary comments from function definitions in Solidity fil…

ff4dd4f

…es for cleaner code.

Refactor function definitions in Solidity files to enhance readabilit…

ad24f84

…y by removing redundant comments.

Delete .changeset/salty-clowns-shake.md

50636ed

Remove deprecated .changeset file and clean up Solidity function comm…

688b242

…ents for improved code clarity.

mrscottyrose mentioned this pull request May 14, 2025

Add NatSpec description to generated functions #59

Open

CoveMB requested a review from a team May 15, 2025 20:51

CoveMB reviewed May 15, 2025

View reviewed changes

.changeset/six-yaks-accept.md

@@ -0,0 +1,2 @@

---

Copy link

Contributor

CoveMB May 15, 2025

Choose a reason for hiding this comment

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

Missing detailed change-set description

ernestognw reviewed May 15, 2025

View reviewed changes

update NatSpec comments to be more insightful

9d40424

CoveMB reviewed May 16, 2025

View reviewed changes

revert back the args

2a54021

CoveMB reviewed May 20, 2025

View reviewed changes

ericglau reviewed May 20, 2025

View reviewed changes

.changeset/six-yaks-accept.md Outdated Show resolved Hide resolved

refactor: update NatSpec comments to be more insightful

eca0d50

CoveMB reviewed May 21, 2025

View reviewed changes

mrscottyrose added 2 commits May 21, 2025 08:40

fix: update mutability from 'pure' to 'view' for ERC721 functions and…

ce61955

… add return type for governor function

refactor: replace internal _burn function with public mint function i…

8e9e569

…n ERC20 contract, updating arguments and comments for clarity

ericglau reviewed May 21, 2025

View reviewed changes

mrscottyrose and others added 2 commits May 22, 2025 05:30

refactor: replace internal _burn function with public mint function i…

cc564f9

…n ERC20 contract, updating arguments and comments for clarity

Update packages/core/solidity/src/erc721.ts

d0b743b

Co-authored-by: Eric Lau <ericglau@outlook.com>

mrscottyrose added 2 commits May 22, 2025 12:20

Merge branch 'fix/59-add-natspec-descriptions' of https://github.com/…

35212b8

…mrscottyrose/contracts-wizard into fix/59-add-natspec-descriptions

refactor: update mutability types, and remove unused internal mint fu…

8cf4a81

…nction

CoveMB reviewed May 26, 2025

View reviewed changes

packages/core/solidity/src/erc20.ts Show resolved Hide resolved

mrscottyrose added 4 commits May 26, 2025 16:54

refactor: update mutability types, and remove unused internal mint fu…

37dc690

…nction

clean duplicate comments and add the removed arguments

946288b

Merge branch 'fix/59-add-natspec-descriptions' of https://github.com/…

5c44e50

…mrscottyrose/contracts-wizard into fix/59-add-natspec-descriptions

refactor: remove outdated NatSpec comments for _checkTokenBridge func…

23b1a74

…tion in ERC20 contract

+                  ],
+                },
+                mint: {

Feat/59 add natspec descriptions #549

Are you sure you want to change the base?

Feat/59 add natspec descriptions #549

Conversation

mrscottyrose commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrscottyrose commented May 15, 2025

Uh oh!

CoveMB commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ernestognw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrscottyrose commented May 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoveMB commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrscottyrose commented May 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrscottyrose commented May 20, 2025

Uh oh!

Uh oh!

mrscottyrose commented May 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoveMB commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrscottyrose commented May 22, 2025

Uh oh!

mrscottyrose commented May 26, 2025

Uh oh!

CoveMB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrscottyrose commented May 26, 2025

Uh oh!

CoveMB commented May 26, 2025

Uh oh!

CoveMB commented May 26, 2025

Uh oh!

mrscottyrose commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrscottyrose commented May 14, 2025 •

edited

Loading

github-actions bot commented May 14, 2025 •

edited

Loading

CoveMB commented May 16, 2025 •

edited

Loading

mrscottyrose commented May 28, 2025 •

edited

Loading