diff --git a/docs/modules/ROOT/pages/accounts.adoc b/docs/modules/ROOT/pages/accounts.adoc index a61011eb8..10f272b83 100644 --- a/docs/modules/ROOT/pages/accounts.adoc +++ b/docs/modules/ROOT/pages/accounts.adoc @@ -22,15 +22,8 @@ A more detailed writeup on the topic can be found on https://perama-v.github.io/ ** <> ** <> * <> -* <> - ** <> - ** <> - ** <> - ** <> - ** <> - ** <> - ** <> - ** <> +* <> +* <> * <> ** <> ** <> @@ -81,14 +74,14 @@ namespace IAccount: # Getters # - func get_nonce() -> (res : felt): + func getNonce() -> (res : felt): end # # Business logic # - func is_valid_signature( + func isValidSignature( hash: felt, signature_len: felt, signature: felt* @@ -110,7 +103,7 @@ end While the interface is agnostic of signature validation schemes, this implementation assumes there's a public-private key pair controlling the Account. That's why the `constructor` function expects a `public_key` parameter to set it. -Since there's also a `set_public_key()` method, accounts can be effectively transferred. +Since there's also a `setPublicKey()` method, accounts can be effectively transferred. === Signer @@ -200,7 +193,7 @@ That's why if you want to change the public key controlling the Account, you wou [,python] ---- -await signer.send_transaction(account, account.contract_address, 'set_public_key', [NEW_KEY]) +await signer.send_transaction(account, account.contract_address, 'setPublicKey', [NEW_KEY]) ---- Or if you want to update the Account's L1 address on the `AccountRegistry` contract, you would @@ -318,25 +311,25 @@ Finally, the `\\__execute__` method takes the `AccountCallArray` and calldata an NOTE: Every transaction utilizes `AccountCallArray`. A single `Call` is treated as a bundle with one message. -== API Specification +== Contract API Specification -This in a nutshell is the Account contract public API: +This in a nutshell is the Account contract public API, followed by the `Account` and `EthAccount` presets: [,cairo] ---- -func get_public_key() -> (res: felt): +func getPublicKey() -> (publicKey: felt): end -func get_nonce() -> (res: felt): +func getNonce() -> (nonce: felt): end -func set_public_key(new_public_key: felt): +func setPublicKey(newPublicKey: felt): end -func is_valid_signature(hash: felt, +func isValidSignature(hash: felt, signature_len: felt, signature: felt* - ) -> (is_valid: felt): + ) -> (isValid: felt): end func __execute__( @@ -349,7 +342,7 @@ func __execute__( end ---- -=== `get_public_key` +=== `getPublicKey` Returns the public key associated with the Account contract. @@ -359,10 +352,10 @@ Returns: [,cairo] ---- -public_key: felt +publicKey: felt ---- -=== `get_nonce` +=== `getNonce` Returns the current transaction nonce for the Account. @@ -375,7 +368,7 @@ Returns: nonce: felt ---- -=== `set_public_key` +=== `setPublicKey` Sets the public key that will control this Account. It can be used to rotate keys for security, change them in case of compromised keys or even transferring ownership of the account. @@ -384,12 +377,12 @@ Parameters: [,cairo] ---- -public_key: felt +publicKey: felt ---- Returns: None. -=== `is_valid_signature` +=== `isValidSignature` This function is inspired by https://eips.ethereum.org/EIPS/eip-1271[EIP-1271] and returns `TRUE` if a given signature is valid, otherwise it reverts. In the future it will return `FALSE` if a given signature is invalid (for more info please check https://github.com/OpenZeppelin/cairo-contracts/issues/327[this issue]). @@ -407,7 +400,7 @@ Returns: [,cairo] ---- -is_valid: felt +isValid: felt ---- NOTE: It may return `FALSE` in the future if a given signature is invalid (follow the discussion on https://github.com/OpenZeppelin/cairo-contracts/issues/327[this issue]). @@ -443,10 +436,61 @@ response_len: felt response: felt* ---- + +== Library API Specification + + +=== `is_valid_signature` + +Returns `TRUE` if a given StarkKey signature is valid, otherwise it reverts. + +Parameters: + +[,cairo] +---- +signature_len: felt +signature: felt* +---- + +Returns: + +[,cairo] +---- +is_valid: felt +---- + +NOTE: It may return `FALSE` in the future if a given signature is invalid (follow the discussion on https://github.com/OpenZeppelin/cairo-contracts/issues/327[this issue]). + + +=== `execute` + +This function calls the target contract with the intended function selector and calldata parameters. It uses `is_valid_signature` to validate the nonce and if the signature matches the message represented by `call_array` and `calldata`. + +Parameters: + +[,cairo] +---- +call_array_len: felt +call_array: AccountCallArray* +calldata_len: felt +calldata: felt* +nonce: felt +---- + +NOTE: The current signature scheme expects a 2-element array like `[sig_r, sig_s]`. + +Returns: + +[,cairo] +---- +response_len: felt +response: felt* +---- + + === `is_valid_eth_signature` Returns `TRUE` if a given signature in the secp256k1 curve is valid, otherwise it reverts. -In the future it will return `FALSE` if a given signature is invalid (for more info please check https://github.com/OpenZeppelin/cairo-contracts/issues/327[this issue]). Parameters: @@ -467,7 +511,7 @@ NOTE: It may return `FALSE` in the future if a given signature is invalid (follo === `eth_execute` -This follows the same idea as the vanilla version of `execute` with the sole difference that signature verification is on the secp256k1 curve. +Same as `execute` with the sole difference that it uses `is_valid_eth_signature` for validation. Parameters: diff --git a/src/openzeppelin/account/IAccount.cairo b/src/openzeppelin/account/IAccount.cairo index 977ad53a7..87540754a 100644 --- a/src/openzeppelin/account/IAccount.cairo +++ b/src/openzeppelin/account/IAccount.cairo @@ -12,18 +12,18 @@ namespace IAccount: # Getters # - func get_nonce() -> (res : felt): + func getNonce() -> (nonce : felt): end # # Business logic # - func is_valid_signature( + func isValidSignature( hash: felt, signature_len: felt, signature: felt* - ) -> (is_valid: felt): + ) -> (isValid: felt): end func __execute__( diff --git a/src/openzeppelin/account/presets/Account.cairo b/src/openzeppelin/account/presets/Account.cairo index 9cfa2e03a..5d5b2b6cc 100644 --- a/src/openzeppelin/account/presets/Account.cairo +++ b/src/openzeppelin/account/presets/Account.cairo @@ -18,8 +18,8 @@ func constructor{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr - }(public_key: felt): - Account.initializer(public_key) + }(publicKey: felt): + Account.initializer(publicKey) return () end @@ -28,23 +28,23 @@ end # @view -func get_public_key{ +func getPublicKey{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr - }() -> (res: felt): - let (res) = Account.get_public_key() - return (res=res) + }() -> (publicKey: felt): + let (publicKey) = Account.get_public_key() + return (publicKey=publicKey) end @view -func get_nonce{ +func getNonce{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr - }() -> (res: felt): - let (res) = Account.get_nonce() - return (res=res) + }() -> (nonce: felt): + let (nonce) = Account.get_nonce() + return (nonce=nonce) end @view @@ -53,8 +53,7 @@ func supportsInterface{ pedersen_ptr: HashBuiltin*, range_check_ptr } (interfaceId: felt) -> (success: felt): - let (success) = ERC165.supports_interface(interfaceId) - return (success) + return ERC165.supports_interface(interfaceId) end # @@ -62,12 +61,12 @@ end # @external -func set_public_key{ +func setPublicKey{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr - }(new_public_key: felt): - Account.set_public_key(new_public_key) + }(newPublicKey: felt): + Account.set_public_key(newPublicKey) return () end @@ -76,7 +75,7 @@ end # @view -func is_valid_signature{ +func isValidSignature{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr, @@ -85,9 +84,9 @@ func is_valid_signature{ hash: felt, signature_len: felt, signature: felt* - ) -> (is_valid: felt): - let (is_valid) = Account.is_valid_signature(hash, signature_len, signature) - return (is_valid=is_valid) + ) -> (isValid: felt): + let (isValid) = Account.is_valid_signature(hash, signature_len, signature) + return (isValid=isValid) end @external diff --git a/src/openzeppelin/account/presets/EthAccount.cairo b/src/openzeppelin/account/presets/EthAccount.cairo index b36e29b2c..23fd4a330 100644 --- a/src/openzeppelin/account/presets/EthAccount.cairo +++ b/src/openzeppelin/account/presets/EthAccount.cairo @@ -16,8 +16,8 @@ func constructor{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr - }(eth_address: felt): - Account.initializer(eth_address) + }(ethAddress: felt): + Account.initializer(ethAddress) return () end @@ -26,23 +26,23 @@ end # @view -func get_eth_address{ +func getEthAddress{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr - }() -> (res: felt): - let (res) = Account.get_public_key() - return (res=res) + }() -> (publicKey: felt): + let (publicKey) = Account.get_public_key() + return (publicKey=publicKey) end @view -func get_nonce{ +func getNonce{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr - }() -> (res: felt): - let (res) = Account.get_nonce() - return (res=res) + }() -> (nonce: felt): + let (nonce) = Account.get_nonce() + return (nonce=nonce) end @view @@ -60,12 +60,12 @@ end # @external -func set_eth_address{ +func setEthAddress{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr - }(new_eth_address: felt): - Account.set_public_key(new_eth_address) + }(newEthAddress: felt): + Account.set_public_key(newEthAddress) return () end @@ -74,7 +74,7 @@ end # @view -func is_valid_signature{ +func isValidSignature{ syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr, @@ -84,9 +84,9 @@ func is_valid_signature{ hash: felt, signature_len: felt, signature: felt* - ) -> (is_valid: felt): - let (is_valid) = Account.is_valid_eth_signature(hash, signature_len, signature) - return (is_valid=is_valid) + ) -> (isValid: felt): + let (isValid) = Account.is_valid_eth_signature(hash, signature_len, signature) + return (isValid=isValid) end @external diff --git a/tests/account/test_Account.py b/tests/account/test_Account.py index 3dd311d54..1db8fe479 100644 --- a/tests/account/test_Account.py +++ b/tests/account/test_Account.py @@ -59,7 +59,7 @@ def account_factory(contract_classes, account_init): async def test_constructor(account_factory): account, *_ = account_factory - execution_info = await account.get_public_key().call() + execution_info = await account.getPublicKey().call() assert execution_info.result == (signer.public_key,) execution_info = await account.supportsInterface(IACCOUNT_ID).call() @@ -122,18 +122,20 @@ async def test_nonce(account_factory): # bump nonce await signer.send_transactions(account, [(initializable.contract_address, 'initialized', [])]) - execution_info = await account.get_nonce().call() - current_nonce = execution_info.result.res + execution_info = await account.getNonce().call() + current_nonce = execution_info.result.nonce # lower nonce await assert_revert( - signer.send_transactions(account, [(initializable.contract_address, 'initialize', [])], current_nonce - 1), + signer.send_transactions( + account, [(initializable.contract_address, 'initialize', [])], current_nonce - 1), reverted_with="Account: nonce is invalid" ) # higher nonce await assert_revert( - signer.send_transactions(account, [(initializable.contract_address, 'initialize', [])], current_nonce + 1), + signer.send_transactions( + account, [(initializable.contract_address, 'initialize', [])], current_nonce + 1), reverted_with="Account: nonce is invalid" ) @@ -148,13 +150,13 @@ async def test_nonce(account_factory): async def test_public_key_setter(account_factory): account, *_ = account_factory - execution_info = await account.get_public_key().call() + execution_info = await account.getPublicKey().call() assert execution_info.result == (signer.public_key,) # set new pubkey - await signer.send_transactions(account, [(account.contract_address, 'set_public_key', [other.public_key])]) + await signer.send_transactions(account, [(account.contract_address, 'setPublicKey', [other.public_key])]) - execution_info = await account.get_public_key().call() + execution_info = await account.getPublicKey().call() assert execution_info.result == (other.public_key,) @@ -166,7 +168,7 @@ async def test_public_key_setter_different_account(account_factory): await assert_revert( signer.send_transactions( bad_account, - [(account.contract_address, 'set_public_key', [other.public_key])] + [(account.contract_address, 'setPublicKey', [other.public_key])] ), reverted_with="Account: caller is not this account" ) @@ -177,9 +179,10 @@ async def test_account_takeover_with_reentrant_call(account_factory): account, _, _, _, attacker = account_factory await assert_revert( - signer.send_transaction(account, attacker.contract_address, 'account_takeover', []), + signer.send_transaction( + account, attacker.contract_address, 'account_takeover', []), reverted_with="Account: no reentrant call" ) - - execution_info = await account.get_public_key().call() + + execution_info = await account.getPublicKey().call() assert execution_info.result == (signer.public_key,) diff --git a/tests/account/test_EthAccount.py b/tests/account/test_EthAccount.py index 8f14b420c..46ed8ee41 100644 --- a/tests/account/test_EthAccount.py +++ b/tests/account/test_EthAccount.py @@ -65,7 +65,7 @@ def account_factory(contract_defs, account_init): async def test_constructor(account_factory): account, *_ = account_factory - execution_info = await account.get_eth_address().call() + execution_info = await account.getEthAddress().call() assert execution_info.result == (signer.eth_address,) execution_info = await account.supportsInterface(IACCOUNT_ID).call() @@ -81,7 +81,7 @@ async def test_execute(account_factory): _, hash, signature = await signer.send_transactions(account, [(initializable.contract_address, 'initialize', [])]) - validity_info, *_ = await signer.send_transactions(account, [(account.contract_address, 'is_valid_signature', [hash, len(signature), *signature])]) + validity_info, *_ = await signer.send_transactions(account, [(account.contract_address, 'isValidSignature', [hash, len(signature), *signature])]) assert validity_info.result.response[0] == TRUE execution_info = await initializable.initialized().call() @@ -89,7 +89,7 @@ async def test_execute(account_factory): # should revert if signature is not correct await assert_revert( - signer.send_transactions(account, [(account.contract_address, 'is_valid_signature', [ + signer.send_transactions(account, [(account.contract_address, 'isValidSignature', [ hash-1, len(signature), *signature])]), reverted_with="Invalid signature" ) @@ -138,8 +138,8 @@ async def test_nonce(account_factory): # bump nonce await signer.send_transactions(account, [(initializable.contract_address, 'initialized', [])]) - execution_info = await account.get_nonce().call() - current_nonce = execution_info.result.res + execution_info = await account.getNonce().call() + current_nonce = execution_info.result.nonce # lower nonce await assert_revert( @@ -166,13 +166,13 @@ async def test_nonce(account_factory): async def test_eth_address_setter(account_factory): account, *_ = account_factory - execution_info = await account.get_eth_address().call() + execution_info = await account.getEthAddress().call() assert execution_info.result == (signer.eth_address,) # set new pubkey - await signer.send_transactions(account, [(account.contract_address, 'set_eth_address', [other.eth_address])]) + await signer.send_transactions(account, [(account.contract_address, 'setEthAddress', [other.eth_address])]) - execution_info = await account.get_eth_address().call() + execution_info = await account.getEthAddress().call() assert execution_info.result == (other.eth_address,) @@ -184,7 +184,7 @@ async def test_eth_address_setter_different_account(account_factory): await assert_revert( signer.send_transactions( bad_account, - [(account.contract_address, 'set_eth_address', [other.eth_address])] + [(account.contract_address, 'setEthAddress', [other.eth_address])] ), reverted_with="Account: caller is not this account" ) @@ -200,5 +200,5 @@ async def test_account_takeover_with_reentrant_call(account_factory): reverted_with="Account: no reentrant call" ) - execution_info = await account.get_eth_address().call() + execution_info = await account.getEthAddress().call() assert execution_info.result == (signer.eth_address,) diff --git a/tests/mocks/AccountReentrancy.cairo b/tests/mocks/AccountReentrancy.cairo index 7efaab6fd..dfea1eead 100644 --- a/tests/mocks/AccountReentrancy.cairo +++ b/tests/mocks/AccountReentrancy.cairo @@ -7,9 +7,9 @@ from starkware.cairo.common.cairo_builtins import HashBuiltin, SignatureBuiltin from starkware.cairo.common.alloc import alloc -const GET_NONCE = 756703644403488948674317127005533987569832834207225504298785384568821383277 +const GET_NONCE = 552903089969425767928229549651117769995883180701982662494390259046237820117 const EXECUTE = 617075754465154585683856897856256838130216341506379215893724690153393808813 -const SET_PUBLIC_KEY = 1307260637166823203998179679098545329314629630090003875272134084395659334905 +const SET_PUBLIC_KEY = 332268845949430430346835224631316185987738351560356300584998172574125127129 @external func account_takeover{ @@ -23,7 +23,7 @@ func account_takeover{ let (empty_calldata: felt*) = alloc() let res = call_contract( contract_address=caller, - function_selector=GET_NONCE, # get_nonce + function_selector=GET_NONCE, # getNonce calldata_size=0, calldata=empty_calldata, ) diff --git a/tests/signers.py b/tests/signers.py index 7c94d9893..2dc727aa6 100644 --- a/tests/signers.py +++ b/tests/signers.py @@ -44,7 +44,7 @@ async def send_transaction(self, account, to, selector_name, calldata, nonce=Non async def send_transactions(self, account, calls, nonce=None, max_fee=0): if nonce is None: - execution_info = await account.get_nonce().call() + execution_info = await account.getNonce().call() nonce, = execution_info.result build_calls = [] @@ -76,7 +76,7 @@ async def send_transaction(self, account, to, selector_name, calldata, nonce=Non async def send_transactions(self, account, calls, nonce=None, max_fee=0): if nonce is None: - execution_info = await account.get_nonce().call() + execution_info = await account.getNonce().call() nonce, = execution_info.result build_calls = []