Improve NatSpec documentation #17

xenoliss · 2024-03-06T15:59:01Z

This PR generally improves NatSpec doc on the contracts defined in src/.

wilsoncusack · 2024-03-06T16:10:32Z

src/CoinbaseSmartWallet.sol

 /// @notice ERC4337-compatible smart contract wallet, based on Solady ERC4337 account implementation
-/// with inspiration from Alchemy's LightAccount and Daimo's DaimoAccount
+///         with inspiration from Alchemy's LightAccount and Daimo's DaimoAccount.


is this auto formatted?

No it's not (i don't think there is auto-formating for that). It's the format I'm use to but I can change it back to start from the beginning of the line.

Honestly, I think I prefer the indents. It's new to me too, but it looks pretty clean.

wilsoncusack · 2024-03-06T16:12:41Z

src/CoinbaseSmartWallet.sol

-    /// @param wrappedSignatureBytes The abi encoded `SignatureWrapper` struct.
-    ///
-    /// @return `true` if the signature verification succeeded, else `false`.
-    function _validateSignature(bytes32 message, bytes calldata wrappedSignatureBytes)


I found wrappedSignatureBytes more clear. What's the thought with this change? I guess now we have the natspec

I changed it to match with the function prototype defined in ERC1271.

wilsoncusack · 2024-03-06T16:13:38Z

src/CoinbaseSmartWalletFactory.sol

 /// @author Coinbase (https://github.com/coinbase/smart-wallet)
 /// @author Solady (https://github.com/vectorized/solady/blob/main/src/accounts/ERC4337Factory.sol)
 contract CoinbaseSmartWalletFactory {
-    /// @dev Address of the ERC4337 implementation.
+    /// @notice Address of the ERC-4337 implementation used to deploy new cloned accounts.


Hm it's not really used to deploy them?

Suggested change

/// @notice Address of the ERC-4337 implementation used to deploy new cloned accounts.

/// @notice Address of the ERC-4337 implementation used as implementation for new accounts.

wilsoncusack · 2024-03-06T16:15:17Z

src/CoinbaseSmartWalletFactory.sol

+    ///
+    /// @dev The account is deployed behind a minimal ERC1967 proxy whose implementation points to
+    ///      the registered ERC-4337 `implementation`.
+    /// @dev The `owners` parameter is a set of addresses and/or public keys depending on the signature


I find this comment confusing, what does 1271 or webauthn have to do with it?

I think we can just say "an array of initial owners of the CoinbaseSmartWallet"

Well I was not sure between keeping things generic or giving the details. I picked the later approach as the contracts are already tightly coupled and if you provide "an array of initial owners of the CoinbaseSmartWallet" without taking care of what they are it might revert.

I think I prefer this expanded description. As a new reader, when I see bytes[] calldata arg in a function declaration, it always begs the question "what's in there?". This description offers threads that can be pulled. I think this is especially relevant and important for public/external methods where a new reader might be starting a fresh read.

wilsoncusack · 2024-03-06T16:16:48Z

src/CoinbaseSmartWalletFactory.sol

-    /// @dev Returns the deterministic address of the account created via `createAccount`.
+    /// @notice Returns the deterministic address of the account created via `createAccount()`.
+    ///
+    /// @param owners The initial set of owners that was / will be provided to `createAccount()`.


Suggested change

/// @param owners The initial set of owners that was / will be provided to `createAccount()`.

/// @param owners The initial set of owners provided to `createAccount()`.

not sure but feels a little cleaer

Not opposed although one could interpret yours as "you must have already created your account"

I think the tense is important to keep consistent. Most of this nat spec is written in the present tense

heh sorry @stevieraykatz so which is this an argument for, my edit?

Yes! I think this qualifies as present tense despite the past-tense "provided"

wilsoncusack · 2024-03-06T16:17:45Z

src/ERC1271.sol

-/// Based on Solady (https://github.com/vectorized/solady/blob/main/src/accounts/ERC1271.sol)
+/// @title ERC-1271 With Cross Account Replay Protection
+///
+/// @notice Abstract ERC-1271 implementation (based on Solady's one) with guards to handle the same


Suggested change

/// @notice Abstract ERC-1271 implementation (based on Solady's one) with guards to handle the same

/// @notice Abstract ERC-1271 implementation (based on Solady's) with guards to handle the same

wilsoncusack · 2024-03-06T16:19:15Z

src/ERC1271.sol

+/// @title ERC-1271 With Cross Account Replay Protection
+///
+/// @notice Abstract ERC-1271 implementation (based on Solady's one) with guards to handle the same
+///         signer being used on multiple accounts, using a nested approach based on EIP-712.


Suggested change

/// signer being used on multiple accounts, using a nested approach based on EIP-712.

/// signer being used on multiple accounts.

wilsoncusack · 2024-03-06T16:20:37Z

src/ERC1271.sol

    ///
-    ///  keccak256("\x19\x01" || someDomainSeparator || hashStruct(someStruct))
+    ///      The OSH must either be:


It's not signed, it's just "original hash"

Not sure the abbreviations are making things more clear.

wilsoncusack · 2024-03-06T16:22:14Z

src/ERC1271.sol

+///         signer being used on multiple accounts, using a nested approach based on EIP-712.
+///
+/// @dev To prevent the same signature from being validated on different accounts owned by the samer signer,
+///      we introduce an anti cross-account-replay layer: the original signed hash (OSH) is wrapped in a


Suggested change

/// we introduce an anti cross-account-replay layer: the original signed hash (OSH) is wrapped in a

/// we introduce an anti cross-account-replay layer: the original hash is input into a new EIP-712-compliant hash. The domain separator of this outer hash contains the chain id and address of this contract, so that it cannot be used on two accounts.

wilsoncusack · 2024-03-06T16:22:56Z

src/ERC1271.sol

+///      we introduce an anti cross-account-replay layer: the original signed hash (OSH) is wrapped in a
+///      `CoinbaseSmartWalletMessage` (CSWM) struct, which is itself hashed using EIP-712.
+///
+///      When hashing the CSWM, the `domainSeparator` used has its `verifyingContract` field set to the address


I think we can simplify, see above.

wilsoncusack · 2024-03-06T16:25:13Z

src/MultiOwnable.sol

+    /// @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,
+    ///      like never run out of owner indexes (practically), at little to no cost.


I don't think using "like" is very formal/good for this context.

wilsoncusack · 2024-03-06T16:25:35Z

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,


Suggested change

/// NOTE: `uint256` rather than a smaller uint because it provides flexibility,

/// NOTE: `uint256` rather than a smaller uint because it provides flexibility, in that we will practically never run out of indices

wilsoncusack · 2024-03-06T16:25:56Z

src/MultiOwnable.sol

+    ///      uint256 id (to economize calldata).
+    ///      NOTE: `uint256` rather than a smaller uint because it provides flexibility,
+    ///      like never run out of owner indexes (practically), at little to no cost.
+    ///      Furthermore on L2, where calldata gas is a concern we should not be charged


This is not a "furthermore" This is explaining the "no cost" in the previous

I saw it as an additional justification to why we picked uint256 and not uint8: because anyway L2s don't charge the 0s.

I actually think the whole statement should be removed. 0 bytes == 0 cost doesn't really add any helpful context.

I agree with @stevieraykatz view on that, I don't think it brings any helpful indication.

wilsoncusack · 2024-03-06T16:26:38Z

src/MultiOwnable.sol

+    ///      Furthermore on L2, where calldata gas is a concern we should not be charged
+    ///      for the extra 0 bytes.
+    ///
+    ///      In the context of checking whether something was signed by an owner this


I find this more confusing than what I had before. We should leave with the motivation for using indices at all.

wilsoncusack · 2024-03-06T16:27:46Z

src/MultiOwnable.sol

 /// @author Coinbase (https://github.com/coinbase/smart-wallet)
 contract MultiOwnable {
-    /// keccak256(abi.encode(uint256(keccak256("coinbase.storage.MultiOwnable")) - 1)) & ~bytes32(uint256(0xff))
+    /// @dev Slot or the `MultiOwnableStorage` struct in storage.


we could mention is erc7201 compliant

wilsoncusack · 2024-03-06T16:28:22Z

src/MultiOwnable.sol

+    /// @notice Helper function to get a storage reference to the `MultiOwnableStorage` struct.
+    ///
+    /// @return $ A storage reference to the `MultiOwnableStorage` struct.
+    function _$() internal pure returns (MultiOwnableStorage storage $) {


is this idiomatic?

Not sure. I personally use _s() and s but I saw you were already using _$() as naming for the slot ( and I find _getMultiOwnableStorage() a bit long to write each time)

Though definitely shorter, if we're claiming to follow ERC-7201 I think we should follow all of its conventions.

stevieraykatz · 2024-03-06T18:07:07Z

src/CoinbaseSmartWallet.sol

 /// @notice ERC4337-compatible smart contract wallet, based on Solady ERC4337 account implementation
-/// with inspiration from Alchemy's LightAccount and Daimo's DaimoAccount
+///         with inspiration from Alchemy's LightAccount and Daimo's DaimoAccount.


Honestly, I think I prefer the indents. It's new to me too, but it looks pretty clean.

stevieraykatz · 2024-03-06T18:22:34Z

src/CoinbaseSmartWalletFactory.sol

+    ///
+    /// @dev The account is deployed behind a minimal ERC1967 proxy whose implementation points to
+    ///      the registered ERC-4337 `implementation`.
+    /// @dev The `owners` parameter is a set of addresses and/or public keys depending on the signature


I think I prefer this expanded description. As a new reader, when I see bytes[] calldata arg in a function declaration, it always begs the question "what's in there?". This description offers threads that can be pulled. I think this is especially relevant and important for public/external methods where a new reader might be starting a fresh read.

stevieraykatz · 2024-03-06T18:44:12Z

src/CoinbaseSmartWalletFactory.sol

-    /// @dev Returns the deterministic address of the account created via `createAccount`.
+    /// @notice Returns the deterministic address of the account created via `createAccount()`.
+    ///
+    /// @param owners The initial set of owners that was / will be provided to `createAccount()`.


I think the tense is important to keep consistent. Most of this nat spec is written in the present tense

stevieraykatz · 2024-03-06T18:52: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 don't think that tying these parameters to the createAccount() method is helpful. This method can be used for any arbitrary set of owners and nonce. I think the defs should be more generic here.

Suggested change

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

/// @notice Returns the deterministic salt for a given `owner` set and `nonce`

stevieraykatz · 2024-03-06T19:05:13Z

src/ERC1271.sol

-
-    /// @dev Validates the signature with ERC1271 return,
-    /// so that this account can also be used as a signer.
+    /// @return result `0x1626ba7e` if validation succeeded, else `0xffffffff`.


Despite referencing EIP-1271 above, it might be clearer to define these "magic numbers" as constants above. If you opt to leave them as comments, then I think it's helpful to describe the derivation in the comment like:

Suggested change

/// @return result `0x1626ba7e` if validation succeeded, else `0xffffffff`.

/// @return result `0x1626ba7e` == bytes4(keccak256("isValidSignature(bytes32,bytes)") upon success, else `0xffffffff`

stevieraykatz · 2024-03-06T19:14:15Z

src/CoinbaseSmartWalletFactory.sol

    address public immutable implementation;

+    /// @notice Reverted when trying to create a new `CoinbaseSmartWallet` account without any owner.


I think I prefer the verb Thrown when describing errors.

I tend to prefer 'reverts' as it is literally what we write (i.e., "revert MyError();") but open to debate. Maybe @wilsoncusack could share his view as well?

stevieraykatz · 2024-03-06T19:15:43Z

src/MultiOwnable.sol

    error InvalidOwnerBytesLength(bytes owner);
+
+    /// @notice Reverted when trying to intialize the contracts owners if a provided owner 32 bytes long


Suggested change

/// @notice Reverted when trying to intialize the contracts owners if a provided owner 32 bytes long

/// @notice Reverted when trying to intialize the contracts owners if a provided owner is 32 bytes long

stevieraykatz · 2024-03-06T19:17:54Z

src/MultiOwnable.sol

    event RemoveOwner(uint256 indexed index, bytes owner);

+    /// @notice Access control modifier ensuring the caller is an owner fo this account (or the account itself).


For two reasons, I think this should be simplified:

It's virtual so the behavior might be changed at some point when overwritten

The method _checkOwner()'s natspec should describe the cases where _checkOwner would pass or fail

Suggested change

/// @notice Access control modifier ensuring the caller is an owner fo this account (or the account itself).

/// @notice Access control modifier ensuring the caller is an authorized owner

stevieraykatz · 2024-03-06T19:20:09Z

src/MultiOwnable.sol

-    function removeOwnerAtIndex(uint8 index) public virtual onlyOwner {
+    /// @notice Removes an owner from the given `index`.
+    ///
+    /// @dev Revert if not owner is registered at `index`.


I think this reads more clearly:

Suggested change

/// @dev Revert if not owner is registered at `index`.

/// @dev Reverts if the owner is not registered at `index`.

stevieraykatz · 2024-03-06T19:22:14Z

src/MultiOwnable.sol

+    /// @notice Helper function to get a storage reference to the `MultiOwnableStorage` struct.
+    ///
+    /// @return $ A storage reference to the `MultiOwnableStorage` struct.
+    function _$() internal pure returns (MultiOwnableStorage storage $) {


Though definitely shorter, if we're claiming to follow ERC-7201 I think we should follow all of its conventions.

xenoliss · 2024-03-07T17:21:32Z

Closing as moved to #18.

xenoliss added 8 commits March 6, 2024 14:21

comment(sw): fix alignment

9b12708

comment(sw-factory): improve NatSpec doc

903fb43

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

ebabc22

comment(erc-1271): improve NatSpec doc

b1df67b

comment(sw): inherit NatSpec doc from ERC1271

d5385ba

comment(multi-owmnable): improve NatSpec doc

8cc1563

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

6d1e241

comment(erc-1271): minor wording changes

fce2bba

wilsoncusack reviewed Mar 6, 2024

View reviewed changes

stevieraykatz reviewed Mar 6, 2024

View reviewed changes

xenoliss mentioned this pull request Mar 7, 2024

Improve NatSpec documentation #18

Merged

xenoliss closed this Mar 7, 2024

	/// @notice Address of the ERC-4337 implementation used to deploy new cloned accounts.
	/// @notice Address of the ERC-4337 implementation used as implementation for new accounts.

	/// @param owners The initial set of owners that was / will be provided to `createAccount()`.
	/// @param owners The initial set of owners provided to `createAccount()`.

	/// @notice Abstract ERC-1271 implementation (based on Solady's one) with guards to handle the same
	/// @notice Abstract ERC-1271 implementation (based on Solady's) with guards to handle the same

	/// signer being used on multiple accounts, using a nested approach based on EIP-712.
	/// signer being used on multiple accounts.

	/// we introduce an anti cross-account-replay layer: the original signed hash (OSH) is wrapped in a
	/// we introduce an anti cross-account-replay layer: the original hash is input into a new EIP-712-compliant hash. The domain separator of this outer hash contains the chain id and address of this contract, so that it cannot be used on two accounts.

	/// NOTE: `uint256` rather than a smaller uint because it provides flexibility,
	/// NOTE: `uint256` rather than a smaller uint because it provides flexibility, in that we will practically never run out of indices

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

	/// @return result `0x1626ba7e` if validation succeeded, else `0xffffffff`.
	/// @return result `0x1626ba7e` == bytes4(keccak256("isValidSignature(bytes32,bytes)") upon success, else `0xffffffff`

		address public immutable implementation;

		/// @notice Reverted when trying to create a new `CoinbaseSmartWallet` account without any owner.

		error InvalidOwnerBytesLength(bytes owner);

		/// @notice Reverted when trying to intialize the contracts owners if a provided owner 32 bytes long

		event RemoveOwner(uint256 indexed index, bytes owner);

		/// @notice Access control modifier ensuring the caller is an owner fo this account (or the account itself).

	/// @dev Revert if not owner is registered at `index`.
	/// @dev Reverts if the owner is not registered at `index`.

Improve NatSpec documentation #17

Improve NatSpec documentation #17

Conversation

xenoliss commented Mar 6, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenoliss Mar 7, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenoliss commented Mar 7, 2024

xenoliss Mar 7, 2024 •

edited

Loading