Improve NatSpec documentation #18

xenoliss · 2024-03-07T17:20:31Z

This PR generally improves NatSpec doc on the contracts and replaces PR #17

…uide

wilsoncusack · 2024-03-08T16:46:20Z

src/MultiOwnable.sol

+    ///      NOTE: `uint256` rather than a smaller uint because it provides flexibility,
+    ///      in that we will practically never run out of indices.
+    ///
+    ///      In the context of checking whether something was signed by an owner this


this is not true in the normal ethereum address case because ecrecover returns the address

wilsoncusack · 2024-03-08T16:47:02Z

src/MultiOwnable.sol

-    mapping(bytes => bool) isOwner;
+    /// @dev Mapping of indices to raw owner bytes, used to idenfitied owners by their
+    ///      uint256 id (to economize calldata).
+    ///      NOTE: `uint256` rather than a smaller uint because it provides flexibility,


I think it's important to explain the "flexibility at little to no cost"

wilsoncusack · 2024-03-08T16:47:36Z

src/MultiOwnable.sol

    function _addOwnerAtIndex(bytes memory owner, uint256 index) internal virtual {
        if (isOwnerBytes(owner)) revert AlreadyOwner(owner);

        _getMultiOwnableStorage().isOwner[owner] = true;
        _getMultiOwnableStorage().ownerAtIndex[index] = owner;

-        emit AddOwner(index, owner);
+        emit AddOwner({index: index, owner: owner});


what's the thought here? We don't use type args elsewhere?

No we don't but I think we should use them more. It helps for readability and increase confidence about writing sound contracts (avoid swapping params).

I actually think they are more cluttering than helpful. We should lean on good variable names to avoid swapping params

yeah I find it redundant here and we should be consistent with our events

I understand your point and agree that it might be redundant, so I will remove them.

However, from my experience, using named parameters has the great benefit of ensuring, without any context about what the function does, nor about its parameter names and types, that the call is most likely correct. When conducting code reviews, I find it much easier to read, as I don't have to hop between the function definition and the function call to make sure the developer wrote what they really intended. You don't have to be a skilled auditor to notice that transfer({to: from, from: to, amount: amount}) is incorrect, which I believe is important if we want to provide secure and foolproof open-source code.

Yes thanks @xenoliss, let's file these away for our style guide discussion, after which we can commit to using the same approach in all places

stevieraykatz · 2024-03-08T23:03:23Z

src/CoinbaseSmartWalletFactory.sol

    function initCodeHash() public view virtual returns (bytes32 result) {
        result = LibClone.initCodeHashERC1967(implementation);
    }

-    /// @dev Returns the salt that will be used for deterministic address
+    /// @notice Returns the salt that will be / was used to deploy the account (using create2).


I think this reads more clearly:

Suggested change

/// @notice Returns the salt that will be / was used to deploy the account (using create2).

/// @notice Returns the deterministic salt for a specific set of `owners` and `nonce`

stevieraykatz · 2024-03-08T23:39:07Z

src/ERC1271.sol

+    /// @notice Validate the `signature` against the given `message`.
+    ///
+    /// @dev MUST be defined by the implementation.
+    /// @dev The `signature` content MIGHT NOT necessarily be the usual (r,s,v) values. It is the responsability


Suggested change

/// @dev The `signature` content MIGHT NOT necessarily be the usual (r,s,v) values. It is the responsability

/// @dev The `signature` content MIGHT NOT necessarily be the usual (r,s,v) values. It is the responsibility

stevieraykatz · 2024-03-08T23:45:37Z

src/MultiOwnable.sol

        0x97e2c6aad4ce5d562ebfaa00db6b9e0fb66ea5d8162ed5b243f51a2e03086f00;

+    /// @notice Reverted when the sender is not an owner and is trying to call a privileged function.


Picking up where the comments ended on the previous pr...

This reads weird to me still. The errors aren't reverting; they are being thrown when reverting. I won't die on this hill but i'm not a fan.

Yeah I think "thrown when" might be better phrasing :)

Alright going to update to "thrown when".

Still thinking about this, given the call is revert error() rather than throw error() maybe @xenoliss had this right 🤔

Although the docs use thrown so let's stick with it

I'm used to Reverted when XXX so I am defiitely biased but when re-reading the docs it is true that errors are not "reverted"; the state is.

stevieraykatz · 2024-03-08T23:49:03Z

src/MultiOwnable.sol

    function _addOwnerAtIndex(bytes memory owner, uint256 index) internal virtual {
        if (isOwnerBytes(owner)) revert AlreadyOwner(owner);

        _getMultiOwnableStorage().isOwner[owner] = true;
        _getMultiOwnableStorage().ownerAtIndex[index] = owner;

-        emit AddOwner(index, owner);
+        emit AddOwner({index: index, owner: owner});


I actually think they are more cluttering than helpful. We should lean on good variable names to avoid swapping params

xenoliss · 2024-03-11T08:31:47Z

I removed the usage of named parameters in the last commit bd548a8. I think we're good to merge.

xenoliss added 18 commits March 7, 2024 17:02

comment(sw): fix alignment

cf578e7

comment(sw-factory): improve NatSpec doc

fe5aafa

comment(sw): refactor top-level contract NatSpec doc

71eca00

comment(erc-1271): improve NatSpec doc

50b2a95

comment(sw): inherit NatSpec doc from ERC1271

420666f

comment(multi-owmnable): update wording

3eb415f

comment(erc-1271-input-gen): improve NatSpec doc

6db3481

comment(erc-1271): update wording

08e7098

comment(sw-factory): update wording

4bc7086

comment(sw-factory): update wording

35b11a1

comment(erc-1271): update wording

1fcc1ff

comment(erc-1271): update wording

8c202ab

comment(erc-1271): update wording

5886310

comment(erc-1271): update wording

0469e14

chore(mutli-ownable): align with ERC-7201 reference implementation

a11b3ba

comment(multi-ownable): fix syntax

9041886

comment(multi-ownable): update wording

2db0a8a

comment(multi-ownable): update wording

3cd97d7

xenoliss mentioned this pull request Mar 7, 2024

Improve NatSpec documentation #17

Closed

xenoliss added 3 commits March 7, 2024 17:33

chore(mutli-ownable): rename constant to upper snake case

16cdb56

chore(mutli-ownable): re-order functions according to soldity style g…

464a91b

…uide

chore(global): remove unnecessary 0 assignments

280d1d5

wilsoncusack reviewed Mar 8, 2024

View reviewed changes

xenoliss added 2 commits March 8, 2024 17:05

chore(mutli-ownable): update wording

d6113a0

chore(mutli-ownable): update wording

9baa4c9

stevieraykatz reviewed Mar 8, 2024

View reviewed changes

xenoliss added 2 commits March 9, 2024 16:42

comment(global): use 'thrown' in error NatSpec doc

be8b956

comment(erc-1271): fix syntax

2a6a9f6

comment(sw-factory): update wording

988c85b

wilsoncusack approved these changes Mar 10, 2024

View reviewed changes

chore(global): revert update to named parameters

bd548a8

wilsoncusack merged commit 299bc7b into coinbase:main Mar 11, 2024
1 check passed

xenoliss deleted the natspec branch March 11, 2024 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve NatSpec documentation #18

Improve NatSpec documentation #18

xenoliss commented Mar 7, 2024

wilsoncusack Mar 8, 2024

wilsoncusack Mar 8, 2024

wilsoncusack Mar 8, 2024

xenoliss Mar 8, 2024

stevieraykatz Mar 8, 2024

wilsoncusack Mar 9, 2024

xenoliss Mar 9, 2024

wilsoncusack Mar 10, 2024

stevieraykatz Mar 8, 2024 •

edited

Loading

stevieraykatz Mar 8, 2024

stevieraykatz Mar 8, 2024

wilsoncusack Mar 9, 2024

xenoliss Mar 9, 2024

wilsoncusack Mar 10, 2024

xenoliss Mar 10, 2024 •

edited

Loading

stevieraykatz Mar 8, 2024

xenoliss commented Mar 11, 2024

	/// @notice Returns the salt that will be / was used to deploy the account (using create2).
	/// @notice Returns the deterministic salt for a specific set of `owners` and `nonce`

	/// @dev The `signature` content MIGHT NOT necessarily be the usual (r,s,v) values. It is the responsability
	/// @dev The `signature` content MIGHT NOT necessarily be the usual (r,s,v) values. It is the responsibility

		0x97e2c6aad4ce5d562ebfaa00db6b9e0fb66ea5d8162ed5b243f51a2e03086f00;

		/// @notice Reverted when the sender is not an owner and is trying to call a privileged function.

Improve NatSpec documentation #18

Improve NatSpec documentation #18

Conversation

xenoliss commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevieraykatz Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenoliss Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenoliss commented Mar 11, 2024

stevieraykatz Mar 8, 2024 •

edited

Loading

xenoliss Mar 10, 2024 •

edited

Loading