Add `nile-account` flag to set Nile artifacts path #315

andrew-fleming · 2022-11-29T09:27:34Z

This PR proposes to add the nile-account flag to account.declare which will create an overriding_path to Nile's precompiled artifacts. This will remove the need to manually set the path for Account declarations.

Nile account declarations will look like this:

out = await account.declare("Account", nile_account=True)

as opposed to something like:

overriding_path = ("env/lib/python3.9/site-packages/nile/artifacts", "env/lib/python3.9/site-packages/nile/artifacts/abis")

out = await account.declare("Account", overriding_path=overriding_path)

Resolves #309.

Update

In the event that an issue arises from the Account contract itself, the PR will now find the artifact in Nile's own artifacts directory. In most cases where errors occur from the contract(s) called from an account, Debug will return the underlined code where the error is coming from in the contracts. The errors referenced specifically in Account will not return the underlined Cairo code because Debug needs the actual .cairo contract to reference.

This PR also proposes to:

Improve the triggered account deployments for methods requiring an account such as deploy, declare, and send. Previously, these deployments would fail but the account would still be registered. This is because the CLI only passed watch_mode to the account method and not the account initialization/deployment.
unregister failed account deployments. Though, it may be useful to keep the account information, I think it's possible that confusion may arise from having the account information when its deployment failed. If it's preferred to leave the data, we can remove the unregister quite easily.

ericnordelo · 2022-11-29T12:12:10Z

Hi Andrew! This approach to declaring looks great!

This PR just needs tests.
Resolves #309.

I just want to mention that 309 is not completely solved by it. We still have the problem with status, which tries to "infer" the artifact of the contract instead of getting it from the deployments.txt file (using execute_call in new starknet_cli.py logic):

nile/src/nile/starknet_cli.py

Lines 76 to 85 in 98ce378

    
           # The rest of the arguments require unique handling 
        
           if kwargs.get("contract_name"): 
        
               base_path = ( 
        
                   kwargs.get("overriding_path") 
        
                   if kwargs.get("overriding_path") 
        
                   else (BUILD_DIRECTORY, ABIS_DIRECTORY) 
        
               ) 
        
               contract = f"{base_path[0]}/{kwargs.get('contract_name')}.json" 
        
               command_args.append("--contract") 
        
               command_args.append(contract)

Every time a transaction is sent through an account (most of the time) status is going to fail to try to get the Account artifact, which is already registered correctly in deployments.txt.

Keeping in mind that status uses the contracts registered in this file, and this file contains the artifacts, I think it makes sense to use the artifacts already registered instead of trying to pass overriding_path down the flow.

EDIT: Why are we registering the artifact in deployments.txt, if we are going to assume they are always inside the nile artifact folder, and if not, we are going to require overriding_path?

martriay · 2022-11-29T21:53:13Z

Agree on reading the deployments file. This file exists for these reasons, it also makes custom account integration easier down the road.

Why are we registering the artifact in deployments.txt, if we are going to assume they are always inside the nile artifact folder

We should use the artifact, but a possible reason to register it if not would be to maintain the file structure/invariants.

andrew-fleming · 2022-11-29T22:39:55Z

Thank you for the early review, @ericnordelo! I have some responses:

We still have the problem with status, which tries to "infer" the artifact of the contract instead of getting it from the deployments.txt file (using execute_call in new starknet_cli.py logic)

This is partially correct^. The issue is actually with Debug in that Debug iterates over the .cairo contracts in order to highlight where the error comes from. The FileNotFoundError: [Errno 2] No such file or directory: 'artifacts/Account.json' is coming from _get_contracts_data with the code below for reference:

def _get_contracts_data(contracts_file, network, addresses):
    file = contracts_file or f"{network}.{DEPLOYMENTS_FILENAME}"
    # contracts_file should already link to compiled contracts and not ABIs
    to_contract = (lambda x: x) if contracts_file else _abi_to_build_path
    contracts = _locate_error_lines_with_abis(file, addresses, to_contract)
    return contracts

Being that we only have the artifacts for Account, this is an issue with the proposed solution. I tested this even further by installing OZ Contracts, compiling Account, and adding the Account.cairo path and it works as I thought.

Every time a transaction is sent through an account (most of the time) status is going to fail to try to get the Account artifact, which is already registered correctly in deployments.txt.

This is not entirely accurate. While it won't be able to iterate over the precompiled contracts, I wouldn't say that the status is going to fail. There's already handling for contracts not found https://github.com/OpenZeppelin/nile/blob/main/src/nile/utils/debug.py#L30-L37

    contracts = _get_contracts_data(contracts_file, network, addresses)

    if not contracts:
        logging.warning(
            "🛑 The transaction was rejected but no contract data is locally "
            "available to improve the error message."
        )
        return error_message

which looks like this:

🚀 Deploying contract
Invoke transaction was sent.
Contract address: 0x04d3f77f305c02d158f159a91e00f4562e8697b7025559aa5f0497446c3bd5de
Transaction hash: 0x7e98b1e34107b963dfced6f57f86b005007bde795423b5ae3d42455a60c48c1
⏳ ️Deployment of contract successfully sent at 0x00dcfac954d8ac11b0de8107520b63c0f1a1bb5928cdec050878370327ce41a4
🧾 Transaction hash: 0x7e98b1e34107b963dfced6f57f86b005007bde795423b5ae3d42455a60c48c1
📦 Registering deployment as my_contract in localhost.deployments.txt
⏳ Querying the network for transaction status...
❌ Transaction status: REJECTED
🛑 The transaction was rejected but no contract data is locally available to improve the error message.
🧾 Error message:
/home/cairo-contracts/env/lib/python3.8/site-packages/starkware/starknet/common/syscalls.cairo:52:5: Error at pc=0:81:
Got an exception while executing a hint.
Cairo traceback (most recent call last):
src/openzeppelin/account/presets/Account.cairo:128:6: (pc=0:731)
src/openzeppelin/account/presets/Account.cairo:143:36: (pc=0:677)
/home/cairo-contracts/src/openzeppelin/account/library.cairo:201:30: (pc=0:291)
/home/cairo-contracts/src/openzeppelin/account/library.cairo:218:19: (pc=0:314)

Error in the called contract (0x41a78e741e5af2fec34b695679bc6891742439f7afb8484ecd7766661ad02bf):
Error at pc=0:32:
Got an exception while executing a hint.
Cairo traceback (most recent call last):
Unknown location (pc=0:174)
Unknown location (pc=0:127)

Traceback (most recent call last):
  ...
    raise get_stark_exception_on_undeclared_contract(class_hash=class_hash)
starkware.starkware_utils.error_handling.StarkException: (500, {'code': <StarknetErrorCode.UNDECLARED_CLASS: 49>, 'message': 'Class with hash 0x72757b6b4922caeb0d478481635494d88d4be03386f3cffbe1858f00974145 is not declared.'})

of course, it'd be preferable to have access to the contracts

I will include a solution for better error handling in cases where the user tries to declare a contract through an undeclared/undeployed sender account. What is not handled, however, is the unregistration of the account through this process (the declaration is nevertheless removed successfully). I'm still looking into the reason.

IMO this whole process highlights why we should prioritize separating account initialization and deployment (#278). With this separation, I believe we'll have stronger control over the account flow and error handling thus better UX.

Forgive me, that's a lot 😅 I'd love some feedback!

ericnordelo · 2022-11-29T23:21:33Z

Thanks for the context @andrew-fleming!

The issue is actually with Debug in that Debug iterates over the .cairo contracts in order to highlight where the error comes from

Yes! My bad. The issue is with debug, not with status, I didn't explain myself well. I said status because debug is a type of status, and is the default that runs (issue only happens if the underlying transaction reverts).

The FileNotFoundError: [Errno 2] No such file or directory: 'artifacts/Account.json' is coming from _get_contracts_data with the code below for reference:

Not sure what we are doing different, but following the same steps provided by Eric in the issue, the traceback of the error I received is this one:

File "/Users/ericnordelo/Documents/Projects/cairo/test_collection/env/lib/python3.9/site-packages/nile/utils/status.py", line 43, in status
   error_message = await debug_message(
 File "/Users/ericnordelo/Documents/Projects/cairo/test_collection/env/lib/python3.9/site-packages/nile/utils/debug.py", line 42, in debug_message
   return await execute_call(
 File "/Users/ericnordelo/Documents/Projects/cairo/test_collection/env/lib/python3.9/site-packages/nile/starknet_cli.py", line 30, in execute_call
   return await capture_stdout(cmd(args=args, command_args=command_args))
 File "/Users/ericnordelo/Documents/Projects/cairo/test_collection/env/lib/python3.9/site-packages/nile/starknet_cli.py", line 47, in capture_stdout
   await func
 File "/Users/ericnordelo/Documents/Projects/cairo/test_collection/env/lib/python3.9/site-packages/starkware/starknet/cli/starknet_cli.py", line 441, in tx_status
   contracts[addr] = Program.load(data=json.load(open(path.strip()))["program"])
FileNotFoundError: [Errno 2] No such file or directory: 'artifacts/Account.json'

Here you can see that the FileNotFoundError error is not coming from _get_contracts_data, but from the execute_call call in line 42. That's why I assumed the issue was coming from the straknet_cli.py design. I'm testing in main branch, maybe this is fixed already in this PR?

This is not entirely accurate. While it won't be able to iterate over the precompiled contracts, I wouldn't say that the status is going to fail. There's already handling for contracts not found

The status is failing already for declare in the issue right? what is the difference with other commands going through accounts? I think is important to discover why your traceback and mine are different in the first place for me to understand this point.

martriay · 2022-11-29T23:48:37Z

i get the same as Eric. so in short, if i understood correctly:

nile custom artifacts are not properly picked up by artifact-based commands, like account's deploy/declare/send. i guess this affects not just account but also ERC20 (eth token) functions?
Andrew accurately raises that once the artifact is in place, debug would still not work since the account’s source code is not available for processing

i assume Andrew's getting a different error either because he added/compiled a new artifact, or because he has a local fix already

andrew-fleming · 2022-11-29T23:59:12Z

@ericnordelo

The issue is with debug, not with status, I didn't explain myself well. I said status because debug is a type of status, and is the default that runs (issue only happens if the underlying transaction reverts).

Haha no worries! This was a tricky issue, and I just wanted to be as precise as possible.

Here you can see that the FileNotFoundError error is not coming from _get_contracts_data, but from the execute_call call in line 42. That's why I assumed the issue was coming from the straknet_cli.py design.

So the assumption is correct in that the error is raised by execute_call. I mentioned that the error is coming from _get_contracts_data because that's where the problem is stemming from (it's just not raised until execute_call). That's why it was tricky because I also assumed we could somehow set the path to the artifacts for instances where they're returning errors.

Also @martriay ^

martriay · 2022-11-30T00:01:11Z

But the error says the missing file is Account.json, not the Cairo one. As you say that's properly handled, so what's the error stemming there? and how does that explain the artifact error we get?

andrew-fleming · 2022-11-30T00:26:52Z

But the error says the missing file is Account.json, not the Cairo one. As you say that's properly handled, so what's the error stemming there? and how does that explain the artifact error we get?

Excellent question. The Account.json is being set in _abi_to_build_path...and setting the path there directly solves that error!

martriay · 2022-12-06T02:58:19Z

src/nile/accounts.py

@@ -26,6 +26,22 @@ def register(pubkey, address, index, alias, network):
        json.dump(accounts, file)


+def unregister(address, network):


could you add tests for this?

martriay · 2022-12-06T03:02:02Z

src/nile/core/account.py

    ):
        """Declare a contract through an Account."""
        max_fee, nonce, _ = await self._process_arguments(max_fee, nonce)

+        if nile_account:
+            assert overriding_path is None


why asserting?

If it's a nile_account, an overriding_path wouldn't make sense because the path to the artifacts lies within the package. The assertion isn't absolutely necessary, but the user might get the wrong idea about where the account is coming from...an error message with the assertion would be helpful

but wouldn't this let users deploy custom accounts?

martriay · 2022-12-06T03:05:26Z

src/nile/cli.py

    await account.declare(
        contract_name,
        alias=alias,
        max_fee=max_fee,
        overriding_path=overriding_path,
        mainnet_token=token,
        watch_mode=watch_mode,
+        nile_account=nile_account,


what about making it a constructor argument and object attribute? i.e. self.is_nile_account

it could be leaner if we eventually need it for other operations, and it would debloat the function's arguments. although right now it's the same since it's just here

We can maybe add this in the future, but I don't think this would work here. The point of nile_account is to direct the path to the Nile artifacts. Really, it's just for declaring a Nile account, so we need a way to adjust the path for this specific type of declaration. In other words, when an account wants to declare a Nile account, use the flag to change the path. For other declarations, Nile assumes the local artifacts/ path.

IMO a better fix would be to iterate both paths checking for the target contract. That way, we don't have to worry about any of this. I already have a working example of this improvement, but for the sake of getting this shipped, I decided not to include it

martriay · 2022-12-06T03:12:29Z

src/nile/utils/debug.py

+        return await execute_call(**execute_args)
+    except FileNotFoundError:
+        # Change path in contracts to locate Nile artifacts
+        contracts = _get_contracts_data(contracts_file, network, addresses, True)


this is using one or the other, right? could it be that the traceback includes both nile and non-nile artifacts? if so, can't both paths be used simultaneously (and avoid re-executing the command)?

Yeah, that should work. I was initially worried about conflicting "Account"s (with how common the contract name is), but that shouldn't be an issue now with the nile_account

we could also have a different name for Nile's

ericnordelo

Looking good @andrew-fleming ! Left a couple of comments. A refactor for the overriding_path handling flow would be helpful I think, but that is out of the scope of this PR.

src/nile/core/account.py

ericnordelo · 2022-12-09T18:37:40Z

src/nile/core/account.py

+
+def set_nile_artifacts_path():
+    """Set path to find Nile's precompiled artifacts."""
+    pt = os.path.dirname(os.path.realpath(__file__)).replace("/core", "")


Any reason to not import pt from nile.common?

If we use it, I would rename pt to nile_root_path or something similar.

Any reason to not import pt from nile.common?

Nope, good call!

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

martriay · 2022-12-10T06:03:42Z

src/nile/common.py

@@ -137,8 +139,7 @@ def get_class_hash(contract_name, overriding_path=None):

 def get_account_class_hash(contract="Account"):
    """Return the class_hash of an Account contract."""
-    pt = os.path.dirname(os.path.realpath(__file__)).replace("/core", "")
-    overriding_path = (f"{pt}/artifacts", f"{pt}/artifacts/abis")
+    overriding_path = (NILE_BUILD_DIR, NILE_ABIS_DIR)


martriay · 2022-12-10T06:10:37Z

src/nile/core/account.py

    ):
        """Declare a contract through an Account."""
        max_fee, nonce, _ = await self._process_arguments(max_fee, nonce)

+        if nile_account:
+            assert overriding_path is None


but wouldn't this let users deploy custom accounts?

martriay

looks good! in the future we should add more tests for account file management, like register

martriay · 2022-12-10T06:20:03Z

tests/commands/test_account.py

    QUERY_VERSION,
    TRANSACTION_VERSION,
    UNIVERSAL_DEPLOYER_ADDRESS,
 )
-from nile.core.account import Account, set_nile_artifacts_path
+from nile.core.account import Account, get_nile_artifacts_path


this seems like a common function, or maybe even just a constant

martriay · 2022-12-10T06:29:18Z

tests/test_accounts.py

@@ -0,0 +1,73 @@
+"""Tests for accounts file."""


then i'd call it tests/test_accounts_file.py

ericnordelo

Looking good to go! Left a small suggestion.

ericnordelo · 2022-12-11T14:51:57Z

src/nile/core/account.py

@@ -297,3 +303,8 @@ def get_counterfactual_address(salt=None, calldata=None, contract="Account"):
        constructor_calldata=calldata,
        deployer_address=0,
    )
+
+
+def get_nile_artifacts_path():


This could be a constant.

ericnordelo

LGTM!

andrew-fleming added 4 commits November 29, 2022 04:16

add nile-account flag

de7a04b

add overriding path to get_class_hash

4996bdc

add overriding_path to tests

d639ffa

fix docstring

f44e22d

add try/except to execute_call

8bbc137

andrew-fleming added 12 commits November 30, 2022 02:42

fix path to locate nile artifacts

c57224c

add _get_contracts_data test

8052418

remove unused var

3b55c9e

Merge remote-tracking branch 'origin' into fix-declare-path

4c482bd

add unregister for accounts

1d879f4

add watch_mode to account inits

72b9c25

unregister account upon account deploy rejection

e8e0edf

remove duplicate func

8c26a0b

fix formatting

7722a76

add declare account test

3419026

fix formatting

9965aa3

remove unused variables

cac7199

andrew-fleming marked this pull request as ready for review December 2, 2022 20:05

martriay reviewed Dec 6, 2022

View reviewed changes

andrew-fleming marked this pull request as draft December 6, 2022 19:20

andrew-fleming added 2 commits December 6, 2022 14:22

add unregister tests, add indent to accounts.json

1991538

add unregister test with wrong address

1086930

andrew-fleming added 2 commits December 7, 2022 13:27

consolidate abi to contract path logic, fix tests

835d310

add assertion error message

5033b53

andrew-fleming marked this pull request as ready for review December 7, 2022 18:52

ericnordelo reviewed Dec 9, 2022

View reviewed changes

andrew-fleming and others added 4 commits December 9, 2022 14:26

Update src/nile/core/account.py

0e2df27

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

add nile root path constants

e96e95f

fix deploy test

96e3e1d

fix formatting

825ff94

andrew-fleming requested review from martriay and ericnordelo December 9, 2022 20:36

martriay reviewed Dec 10, 2022

View reviewed changes

martriay previously approved these changes Dec 10, 2022

View reviewed changes

ericnordelo previously approved these changes Dec 11, 2022

View reviewed changes

andrew-fleming added 2 commits December 12, 2022 01:20

define NILE_ARTIFACTS_PATH as constant

28ae116

rename test_accounts.py

424bc90

andrew-fleming dismissed stale reviews from ericnordelo and martriay via 424bc90 December 12, 2022 06:21

ericnordelo approved these changes Dec 12, 2022

View reviewed changes

martriay merged commit a21b2fb into OpenZeppelin:main Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `nile-account` flag to set Nile artifacts path #315

Add `nile-account` flag to set Nile artifacts path #315

andrew-fleming commented Nov 29, 2022 •

edited

Loading

ericnordelo commented Nov 29, 2022 •

edited

Loading

martriay commented Nov 29, 2022

andrew-fleming commented Nov 29, 2022 •

edited

Loading

ericnordelo commented Nov 29, 2022 •

edited

Loading

martriay commented Nov 29, 2022

andrew-fleming commented Nov 29, 2022

martriay commented Nov 30, 2022

andrew-fleming commented Nov 30, 2022

martriay Dec 6, 2022

martriay Dec 6, 2022

andrew-fleming Dec 6, 2022

martriay Dec 10, 2022

martriay Dec 6, 2022

andrew-fleming Dec 7, 2022

martriay Dec 6, 2022

andrew-fleming Dec 6, 2022

martriay Dec 6, 2022

ericnordelo left a comment

ericnordelo Dec 9, 2022

ericnordelo Dec 9, 2022

andrew-fleming Dec 9, 2022

martriay Dec 10, 2022

martriay Dec 10, 2022

martriay left a comment

martriay Dec 10, 2022

martriay Dec 10, 2022

ericnordelo left a comment

ericnordelo Dec 11, 2022

ericnordelo left a comment

		@@ -26,6 +26,22 @@ def register(pubkey, address, index, alias, network):
		json.dump(accounts, file)


		def unregister(address, network):

Add nile-account flag to set Nile artifacts path #315

Add nile-account flag to set Nile artifacts path #315

Conversation

andrew-fleming commented Nov 29, 2022 • edited Loading

Update

ericnordelo commented Nov 29, 2022 • edited Loading

martriay commented Nov 29, 2022

andrew-fleming commented Nov 29, 2022 • edited Loading

ericnordelo commented Nov 29, 2022 • edited Loading

martriay commented Nov 29, 2022

andrew-fleming commented Nov 29, 2022

martriay commented Nov 30, 2022

andrew-fleming commented Nov 30, 2022

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

ericnordelo 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

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

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

Add `nile-account` flag to set Nile artifacts path #315

Add `nile-account` flag to set Nile artifacts path #315

andrew-fleming commented Nov 29, 2022 •

edited

Loading

ericnordelo commented Nov 29, 2022 •

edited

Loading

andrew-fleming commented Nov 29, 2022 •

edited

Loading

ericnordelo commented Nov 29, 2022 •

edited

Loading