Update documentation #214

andrew-fleming · 2022-03-09T20:21:32Z

This PR updates ERC20, Extensibility, and Account docs.

In the ERC20 docs, this PR:

updates Extensibility section to include the extensibility pattern
adds an example to construct an exposed method with extended logic
proposes to add a Presets section
fixes some of the documentation in regard to ERC20 compliance

In the Extensibility docs, this PR:

updates Presets
fixes code snippet

In the Account docs, this PR:

updates all references to the interface (mainly __execute__ and its params)
adds documentation for multicall
provides an example of multicall
fixes Extensibility section

This resolves #206.

martriay

Very good improvements!

docs/Account.md

martriay · 2022-03-13T19:47:00Z

docs/Account.md

+- `max_fee` is the maximum fee a user will pay
+- `version` is a fixed number which is used to invalidate old transactions
+
+This `MultiCall` message is consumed by the `__execute__` method, which acts as a single entrypoint for all user interaction with any contract, including managing the account contract itself. That's why if you want to change the public key controlling the Account, you would send a transaction targeting the very Account contract:


Note that __execute__ doesn't take a MultiCall but it builds one (until starknet allows for pointers on struct arrays)! It expects this instead:

call_array_len: felt, call_array: AccountCallArray*, calldata_len: felt, calldata: felt*, nonce: felt

I think it's worth documenting the actual interface

martriay · 2022-03-14T04:08:34Z

docs/Account.md

@@ -265,11 +300,13 @@ response: felt*

 Certain contracts like ERC721 require a means to differentiate between account contracts and non-account contracts. For a contract to declare itself as an account, it should implement [ERC165](https://eips.ethereum.org/EIPS/eip-165) as proposed in [#100](https://github.com/OpenZeppelin/cairo-contracts/discussions/100). To be in compliance with ERC165 specifications, the idea is to calculate the XOR of `IAccount`'s EVM selectors (not StarkNet selectors). The resulting magic value of `IAccount` is 0x50b70dcb.

-Our ERC165 integration on StarkNet is inspired by OpenZeppelin's Solidity implementation of [ERC165Storage](https://docs.openzeppelin.com/contracts/4.x/api/utils#ERC165Storage) which stores the interfaces that the implementing contract supports. In the case of account contracts, querying `supportsInterface` of an account's address with the `IAccount` magic value should return true (meaning `1` in Cairo).
+Our ERC165 integration on StarkNet is inspired by OpenZeppelin's Solidity implementation of [ERC165Storage](https://docs.openzeppelin.com/contracts/4.x/api/utils#ERC165Storage) which stores the interfaces that the implementing contract supports. In the case of account contracts, querying `supportsInterface` of an account's address with the `IAccount` magic value should return `TRUE`.


I'd add a link to our constants' doc, someone might think there's such a native value in Cairo

martriay · 2022-03-14T04:10:01Z

docs/Account.md

-There's no clear contract extensibility pattern for Cairo smart contracts yet. In the meantime the best way to extend our contracts is copypasting and modifying them at your own risk. Since `execute` relies on it, we suggest changing how `is_valid_signature` works to explore different signature validation schemes such as multisig, or some guardian logic like in [Argent's account](https://github.com/argentlabs/argent-contracts-starknet/blob/de5654555309fa76160ba3d7393d32d2b12e7349/contracts/ArgentAccount.cairo).
+Account contracts can be extended by following the [extensibility pattern](../docs/Extensibility.md#the-pattern). The basic idea behind integrating the pattern is to import the requisite methods from the Account library and incorporate the extended logic thereafter. The Account library is constructed to accomodate the current validation scheme; however, new schemes may require amending or adding methods in the library. 
+
+Since `__execute__` relies on it, we suggest changing how `is_valid_signature` works to explore different signature validation schemes such as multisig, some guardian logic like in [Argent's account](https://github.com/argentlabs/argent-contracts-starknet/blob/de5654555309fa76160ba3d7393d32d2b12e7349/contracts/ArgentAccount.cairo), or even [Ethereum signatures](https://github.com/OpenZeppelin/cairo-contracts/issues/161).


This is not the case anymore since (except for the above paragraph) we don't suggest changing the libraries but using them, and/or changing the presets. I'd say the Account library is not yet ready for extensibility, or at least I'm under that impression until we experiment a bit more with it. Not sure what to do about this section in the meantime, thoughts?

Yeah, I was having trouble with writing this section, as we'll need to refactor some of the library's methods to allow for new schemes. That's how it looks to me with is_valid_signature, anyway. For now, I suppose we can keep it simple and just say something like: "Account extensibility is not yet available." Maybe make an issue and leave a link here?

Yes we should write it taking that into account, maybe making an easy-to-replace paragraph when we make the changes. I wouldn't say it's not available either, just maybe mentioning that there's a single library/preset and we're looking for feedback/new presets.

martriay · 2022-03-14T04:12:27Z

docs/ERC20.md

@@ -124,13 +127,47 @@ await signer.send_transaction(account, erc20.contract_address, 'transfer', [reci

 ## Extensibility

-There's no clear contract extensibility pattern for Cairo smart contracts yet. In the meantime the best way to extend our contracts is copypasting and modifying them at your own risk.
+ERC20 contracts can be extended by following the [extensibility pattern](../docs/Extensibility.md#the-pattern). The basic idea behind integrating the pattern is to import the requisite ERC20 methods from the ERC20 library and incorporate the extended logic thereafter. For example, let's say you wanted to implement a pausing mechanism. The contract should first import the ERC20 methods (and the extended logic if it's in a separate contract). Next, the contract should expose the methods with the extended logic therein like this:


(and the extended logic if it's in a separate contract)

I didn't get that part, what do you mean?

Ah I meant library, not contract. I was trying to convey an example like how ERC20_Pausable combines functions from two libraries (ERC20 and pausable) into the exposed functions—as opposed to importing from just one library and having the extended behavior coded in the contract itself. I don't think I phrased this well :) I'll edit

Definitely! I would try to convey that extensibility can be custom as well, not just library-based.

docs/ERC20.md

martriay · 2022-03-14T04:14:01Z

docs/ERC20.md

+### ERC20 (basic)
+
+The `ERC20` preset offers a quick and easy setup for deploying a basic ERC20 token.
+
+### ERC20_Mintable
+
+The `ERC20_Mintable` preset allows the contract owner to mint new tokens. 
+
+### ERC20_Pausable
+
+The `ERC20_Pausable` preset allows the contract owner to pause/unpause all state-modifying methods i.e. `transfer`, `approve`, etc. This preset proves useful for scenarios such as preventing trades until the end of an evaluation period and having an emergency switch for freezing all token transfers in the event of a large bug.


Maybe adding links to the presets for inspection?

Co-authored-by: Martín Triay <[email protected]>

docs/ERC20.md

andrew-fleming added 2 commits March 8, 2022 10:04

fix compatibility section

f990f9c

fix extensibility, add presets

8642cfc

andrew-fleming marked this pull request as draft March 9, 2022 21:00

andrew-fleming added 2 commits March 9, 2022 16:28

update extensibility presets

7935db3

fix code snippet

f7f9276

andrew-fleming changed the title ~~Fix erc20 docs~~ Update documentation Mar 10, 2022

andrew-fleming added 2 commits March 10, 2022 01:47

update api

d9bc023

update extensibility section

b1ce8d8

andrew-fleming marked this pull request as ready for review March 10, 2022 18:23

martriay added the documentation Improvements or additions to documentation label Mar 13, 2022

martriay suggested changes Mar 14, 2022

View reviewed changes

andrew-fleming and others added 4 commits March 17, 2022 12:43

Apply suggestions from code review

3147fdf

Co-authored-by: Martín Triay <[email protected]>

add preset links, fix extensibility section

c702153

fix execute section and example, add constants link

efa2b7b

fix extensibility section

8aafc7b

andrew-fleming marked this pull request as draft March 22, 2022 19:18

andrew-fleming marked this pull request as ready for review March 22, 2022 21:01

martriay approved these changes Mar 25, 2022

View reviewed changes

docs/ERC20.md Outdated Show resolved Hide resolved

Update docs/ERC20.md

fda9f9d

martriay merged commit 5bfe5f4 into OpenZeppelin:main Mar 25, 2022

andrew-fleming deleted the fix-erc20-docs branch March 25, 2022 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update documentation #214

Update documentation #214

andrew-fleming commented Mar 9, 2022 •

edited

Loading

martriay left a comment

martriay Mar 13, 2022

martriay Mar 14, 2022

martriay Mar 14, 2022

andrew-fleming Mar 14, 2022

martriay Mar 15, 2022 •

edited

Loading

martriay Mar 14, 2022

andrew-fleming Mar 14, 2022

martriay Mar 15, 2022

martriay Mar 14, 2022

Update documentation #214

Update documentation #214

Conversation

andrew-fleming commented Mar 9, 2022 • edited Loading

martriay left a 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

martriay Mar 15, 2022 • 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

andrew-fleming commented Mar 9, 2022 •

edited

Loading

martriay Mar 15, 2022 •

edited

Loading