Bump to cairo v0.10 #449

andrew-fleming · 2022-09-03T07:59:33Z

This PR proposes to:

resolve bump to Cairo v0.10.0 #403
- update the syntax of all Cairo contract
- refactor Account.execute, separate validation with __validate__ and __validate_declare__
- check that version == 1
- remove nonce handling
update Account interface
use new IAccount selector id for ERC165
change Account's dynamic ERC165 implementation to static (resolves Make Account 165 checks static #383)
migrate Account methods to camelCase (resolves Migrate Account to use camelCase #382)
fix comments in safemath (resolves Wrong use of "subtrahend" and "minuend" in comments #442)
change old function res return values to a more meaningful name

On top of the many changes, the new Cairo version's testing framework hasn't accounted for all the updates. This PR proposes to account for this by refactoring the MockSigner. Some things to note:

fetching the return values for signer calls is different i.e. return_val.result.response is now return_val.call_info.retdata[]
assert_event_emitted makes assertions against the OrderedEvent object instead of Event
signers.py includes a local mock Nile. This should be removed once Nile supports Cairo v0.10
cairo-nile is removed as a dep in tox. This should also be removed upon its update.

martriay

One thing that's not documented on the issue is updating documentation. Considering we aim to safely launch as soon as possible I suggest to leave it outside of scope.

src/openzeppelin/access/ownable/library.cairo

src/openzeppelin/access/accesscontrol/library.cairo

ericnordelo

Hi Andrew, I left a couple of question and comments.

src/openzeppelin/account/library.cairo

tests/mocks/AccessControl.cairo

ericnordelo · 2022-09-06T11:42:14Z

tests/mocks/ERC165.cairo

-    let (success) = ERC165.supports_interface(interfaceId)
-    return (success)
-end
+func supportsInterface{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(


Should we refactor to avoid returning tuples but single values in cases like this? (0.10 introduction of single return values)

Ahh yes, good call. This is just what cairo-migrate spit out. I'll update

tests/mocks/ERC721SafeMintableMock.cairo

src/openzeppelin/access/accesscontrol/library.cairo

src/openzeppelin/access/ownable/library.cairo

src/openzeppelin/account/library.cairo

martriay · 2022-09-07T01:34:59Z

src/openzeppelin/account/presets/Account.cairo

+    let (res) = Account.get_public_key();
+    return (res=res);


Suggested change

let (res) = Account.get_public_key();

return (res=res);

return Account.get_public_key();

let's try to return a single value. if not, at least not res:

Suggested change

let (res) = Account.get_public_key();

return (res=res);

let (publicKey) = Account.get_public_key();

return (publicKey);

martriay · 2022-09-07T01:37:19Z

src/openzeppelin/account/presets/EthAccount.cairo

+    let (res) = Account.get_public_key();
+    return (res=res);


same as the other

juniset · 2022-09-07T17:18:00Z

src/openzeppelin/utils/constants/library.cairo

-# Account
-const IACCOUNT_ID = 0xf10dbd44
+// Account
+const IACCOUNT_ID = 0xf10dbd44;


We should update that value according to the new IAccount interface.

Yes, good call.

@juniset @martriay we're including isValidSignature in the interface, correct? If so, this is how it looks. Let me know if there are any objections.

interface IAccount { function isValidSignature(bytes32, bytes32, bytes32) external; // hash, r, s function __execute__(address, bytes4, bytes calldata) external; // to, selector, calldata function __validate__(address, bytes4, bytes calldata) external; // to, selector, calldata function __validate_declare__(bytes32) external; // class hash }

@andrew-fleming I don't think that properly represents the IAccount interface. We need to take into account that we can have multiple signatures in isValidSignature and multicalls in __execute__ and __validate__.

I would propose to use

interface IAccount { struct Call { address to; bytes4 selector; uint256 data; } function isValidSignature(bytes32, bytes32[] calldata) external; function __execute__(Call[] calldata) external; function __validate__(Call[] calldata) external; function __validate_declare__(bytes32) external; } contract Selector { // returns 0x56ad812f function calculateSelector() public pure returns (bytes4) { IAccount i; return i.isValidSignature.selector ^ i.__execute__.selector ^ i.__validate__.selector ^ i.__validate_declare__.selector; } }

which gives IACCOUNT_ID = 0x56ad812f.

What do you think?

By the way, shouldn't we include supportsInterface to the IAccount interface? It's the only way for a contract to detect that an address is an account hence it should be mandatory.

You're right.But I think it should be bytes then.

Perfect. And I just spoke to @martriay and we're all in agreement with adding supportsInterface to IAccount; therefore:

interface IAccount { struct Call { address to; bytes4 selector; bytes data; } function supportsInterface(bytes4) external; function isValidSignature(bytes32, bytes32[] calldata) external; function __execute__(Call[] calldata) external; function __validate__(Call[] calldata) external; function __validate_declare__(bytes32) external; } contract Selector { // returns 0xa66bd575 function calculateSelector() public pure returns (bytes4) { IAccount i; return i.supportsInterface.selector^ i.isValidSignature.selector ^ i.__execute__.selector ^ i.__validate__.selector ^ i.__validate_declare__.selector; } }

which gives us IACCOUNT = 0xa66bd575. What does everyone think?

Thanks @andrew-fleming , that looks good to me!

Awesome! I still have two doubts:

should bytes4 be bytes32 considering that selectors are way longer (1 felt) in Cairo?

our current interface doesn't actually take Call[] because we can't handle arrays of complex structs yet. I guess we want to be future proof but do we agree this isn't the current interface?

other than that, LGTM

should bytes4 be bytes32 considering that selectors are way longer (1 felt) in Cairo?

I agree that bytes32 is a more accurate type for SN selectors; however, bytes4 seems more compliant with EIP165. I vote for keeping bytes4

our current interface doesn't actually take Call[] because we can't handle arrays of complex structs yet. I guess we want to be future proof but do we agree this isn't the current interface?

And yes, I agree the proposed interface is the interface-to-be, not the interface right now

martriay

getting there 🥺

src/openzeppelin/token/erc721/library.cairo

src/openzeppelin/account/library.cairo

src/openzeppelin/token/erc20/presets/ERC20.cairo

martriay

Super small changes requested, otherwise good to go!

martriay · 2022-09-11T21:10:43Z

src/openzeppelin/token/erc20/presets/ERC20Upgradeable.cairo

+    ERC20.transfer_from(sender, recipient, amount);
+    return ERC20.transfer_from(sender, recipient, amount);


doublespend! is this passing the tests?

src/openzeppelin/account/presets/Account.cairo

Co-authored-by: Martín Triay <martriay@gmail.com>

ericnordelo

@martriay @andrew-fleming I left some comments but overall the PR LGTM (I know is already merged, but I added the last review as requested by @martriay).

ericnordelo · 2022-09-12T10:57:53Z

src/openzeppelin/account/library.cairo

+    //
+
+    func initializer{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(
+        _public_key: felt


This underscore at the beginning of the argument name has some purpose?

Good catch. It did serve a purpose early on when public_key itself was a storage variable

src/openzeppelin/account/library.cairo

ericnordelo · 2022-09-12T11:17:24Z

src/openzeppelin/account/presets/Account.cairo

-#
-# Getters
-#
+func constructor{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(


We changing the implicit arguments to lay in one line (instead of multiline as before) implies that we will follow the Cairo provided formatter to format the code?

As of now, this is temporary^. We just wanted to ship the cairo v0.10 update

ericnordelo · 2022-09-12T11:22:07Z

src/openzeppelin/account/library.cairo

+        ecdsa_ptr: SignatureBuiltin*,
+        range_check_ptr,
+    }(hash: felt, signature_len: felt, signature: felt*) -> (is_valid: felt) {
+        let (_public_key) = Account_public_key.read();


Another leading underscore.

ericnordelo · 2022-09-12T11:25:57Z

src/openzeppelin/account/presets/Account.cairo

-#
-# Business logic
-#
+func setPublicKey{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(


This method can make the contract unusable if a wrong public key is set by mistake. Maybe it would be good to add a two steps process allowing to claim the new public key before removing the old one? (Similar to the ownership two-steps transfers process in Solidity contracts).

Yes, agreed. If we're all in agreement, it's probably better suited for the next release

Created an issue for this #469

ericnordelo · 2022-09-12T11:48:02Z

src/openzeppelin/account/presets/EthAccount.cairo

-    Account.set_public_key(new_eth_address)
-    return ()
-end
+func setEthAddress{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(


Same comment for two steps process as mentioned in Account.cairo.

Mentioned in #469

ericnordelo · 2022-09-12T12:15:32Z

src/openzeppelin/token/erc20/presets/ERC20Upgradeable.cairo

-#
-# Getters
-#
+func upgrade{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(


Just a reminder of the upgradeAndCall suggestion to keep the atomicity if the new implementation needs initialization (this is in general for the upgrade function in upgradeable contracts).

ericnordelo · 2022-09-12T12:19:50Z

src/openzeppelin/token/erc721/library.cairo

@@ -14,549 +14,458 @@ from openzeppelin.introspection.erc165.IERC165 import IERC165
 from openzeppelin.security.safemath.library import SafeUint256
 from openzeppelin.token.erc721.IERC721Receiver import IERC721Receiver
 from openzeppelin.utils.constants.library import (
-    IERC721_ID, IERC721_METADATA_ID, IERC721_RECEIVER_ID, IACCOUNT_ID


A reminder for myself to double check Interface IDs.

ericnordelo · 2022-09-12T12:26:58Z

src/openzeppelin/upgrades/library.cairo

+    func initializer{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(
+        proxy_admin: felt
+    ) {
+        let (initialized) = Proxy_initialized.read();


Shouldn't we be using here the initializable library instead? I think importing libraries from libraries is not against the Extensibility Pattern right?

Shouldn't we be using here the initializable library instead?

Agreed

I think importing libraries from libraries is not against the Extensibility Pattern right?

Correct :)

Created an issue for this #470

ericnordelo · 2022-09-12T12:28:17Z

src/openzeppelin/utils/constants/library.cairo


-# ERC165
-const IERC165_ID = 0x01ffc9a7


The remainder for double-checking Interfaces Ids is better left here.

andrew-fleming added 3 commits September 3, 2022 03:56

refactor account contracts

2573ba2

update cairo syntax

7361266

bump cairo in tox

4da51b8

martriay reviewed Sep 5, 2022

View reviewed changes

src/openzeppelin/access/ownable/library.cairo Show resolved Hide resolved

src/openzeppelin/access/accesscontrol/library.cairo Show resolved Hide resolved

martriay requested review from martriay and ericnordelo September 5, 2022 15:27

andrew-fleming added 2 commits September 5, 2022 13:57

migrate mocks

b5c90e8

check version in account

87fea7c

milancermak mentioned this pull request Sep 6, 2022

Define (or select) and follow a code format for cairo contracts #454

Closed

ericnordelo reviewed Sep 6, 2022

View reviewed changes

martriay mentioned this pull request Sep 7, 2022

Add erc1155 #290

Closed

martriay suggested changes Sep 7, 2022

View reviewed changes

andrew-fleming added 2 commits September 7, 2022 03:28

fix return vals

c7f4c8f

remove unused imports

f76e7b1

martriay mentioned this pull request Sep 7, 2022

Cairo V0.10 #457

Closed

juniset reviewed Sep 7, 2022

View reviewed changes

andrew-fleming added 12 commits September 8, 2022 06:03

update syntax

205756e

add static erc165 support

1c22acc

fix return val name

edc3ba0

update IACCOUNT_ID

2a7672f

tmp comment out nile dep

b383fec

fix syntax

b111e1c

refactor event assertion, fix cached_contract

8006a3e

update signers, add mock nile

91c8f78

remove unused function

08b7711

fix tests

ff1d70d

update id

470f489

update account id

3c9b67a

martriay mentioned this pull request Sep 8, 2022

Migrate account presets to camelCase #446

Closed

update interface

2731a3f

andrew-fleming added 13 commits September 10, 2022 02:30

simplify view funcs, return success from lib

499b387

simplify return

b510780

remove kwarg

d49305c

simplify presets

09e39f8

update return var

38d78d5

add from_address to events

7f67464

remove unused import

be8a45d

simplify getters

c1418e2

simplify getters

a6a0cae

simplify getters

f179ca0

simplify getters

453fd43

fix return var

c1339f3

simplify getters

1dc7904

martriay reviewed Sep 10, 2022

View reviewed changes

andrew-fleming added 2 commits September 10, 2022 16:35

simplify externals

112df72

bind return vals

49d58d0

martriay suggested changes Sep 11, 2022

View reviewed changes

andrew-fleming and others added 2 commits September 11, 2022 18:28

Update src/openzeppelin/account/presets/Account.cairo

ca02a1a

Co-authored-by: Martín Triay <martriay@gmail.com>

fix and test transferFrom

31508a9

martriay approved these changes Sep 11, 2022

View reviewed changes

martriay merged commit a9894b4 into OpenZeppelin:main Sep 11, 2022

andrew-fleming deleted the cairo-v0.10 branch September 12, 2022 03:39

ericnordelo reviewed Sep 12, 2022

View reviewed changes

andrew-fleming mentioned this pull request Sep 12, 2022

Refactor tests for better preset coverage #464

Closed

This was referenced Sep 30, 2022

Update documentation to Cairo v0.10 #472

Merged

Import utils funcs from Nile/remove funcs from utils.py #450

Merged

martriay mentioned this pull request Nov 16, 2022

update ERC165_ACCOUNT_INTERFACE_ID argentlabs/argent-contracts-starknet#75

Closed

sgc-code mentioned this pull request Apr 24, 2023

Update IACCOUNT_ID to match the interface for Cairo 1 #612

Closed

martriay mentioned this pull request Apr 26, 2023

Migrate constants #611

Merged

3 tasks

andrew-fleming mentioned this pull request May 11, 2023

Migrate account #616

Closed

3 tasks

	let (res) = Account.get_public_key();
	return (res=res);
	return Account.get_public_key();

		ERC20.transfer_from(sender, recipient, amount);
		return ERC20.transfer_from(sender, recipient, amount);

Bump to cairo v0.10 #449

Bump to cairo v0.10 #449

Conversation

andrew-fleming commented Sep 3, 2022 • edited Loading

martriay left a comment

Choose a reason for hiding this comment

ericnordelo left a comment • 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

juniset Sep 8, 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

Choose a reason for hiding this comment

martriay left a comment • edited Loading

Choose a reason for hiding this comment

martriay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo left a comment • 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

ericnordelo Sep 12, 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

ericnordelo Sep 12, 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

Choose a reason for hiding this comment

andrew-fleming commented Sep 3, 2022 •

edited

Loading

ericnordelo left a comment •

edited

Loading

juniset Sep 8, 2022 •

edited

Loading

martriay left a comment •

edited

Loading

ericnordelo left a comment •

edited

Loading

ericnordelo Sep 12, 2022 •

edited

Loading

ericnordelo Sep 12, 2022 •

edited

Loading