nile icon indicating copy to clipboard operation
nile copied to clipboard

Improve tests for account.declare

Open ericglau opened this issue 3 years ago • 2 comments

The below was fixed by https://github.com/OpenZeppelin/nile/pull/122 in main, but the tests should still be investigated/improved to avoid this kind of error going forward.


This occurs in main. Does not occur in a release.

Running the command nile declare PKEY1 contract causes the following error:

🚀 Declaring contract
⏳ Successfully sent declaration of contract as 0x0f28592e02fb10ae73e235b5959bd18297efaa4333ded153bdbefc785a243ef
🧾 Transaction hash: 0x3209fb31d79a4e53d3fccdca9f23b60d89734ded57d479a194589aa084ac073
Traceback (most recent call last):
  File "/Users/eric/git/nileproj2/env/bin/nile", line 8, in <module>
    sys.exit(cli())
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/Users/eric/git/nile/src/nile/cli.py", line 117, in declare
    account.declare(
  File "/Users/eric/git/nile/src/nile/core/account.py", line 116, in declare
    return declare(
  File "/Users/eric/git/nile/src/nile/core/declare.py", line 45, in declare
    deployments.register_class_hash(padded_hash, network, alias)
  File "/Users/eric/git/nile/src/nile/deployments.py", line 85, in register_class_hash
    hash = hex(hash)
TypeError: 'str' object cannot be interpreted as an integer

The same occurs from the following script using NRE:

from nile.core.account import Account

def run(nre):
    signer = "PKEY1"
    account = Account(signer, nre.network)
    account.declare("contract")

which causes error:

Traceback (most recent call last):
  File "/Users/eric/git/nileproj2/env/bin/nile", line 8, in <module>
    sys.exit(cli())
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/eric/git/nileproj2/env/lib/python3.9/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/Users/eric/git/nile/src/nile/cli.py", line 83, in run
    run_command(path, network)
  File "/Users/eric/git/nile/src/nile/core/run.py", line 14, in run
    script.run(nre)
  File "scripts/declare.py", line 7, in run
    account.declare("contract")
  File "/Users/eric/git/nile/src/nile/core/account.py", line 116, in declare
    return declare(
  File "/Users/eric/git/nile/src/nile/core/declare.py", line 45, in declare
    deployments.register_class_hash(padded_hash, network, alias)
  File "/Users/eric/git/nile/src/nile/deployments.py", line 85, in register_class_hash
    hash = hex(hash)
TypeError: 'str' object cannot be interpreted as an integer

ericglau avatar Nov 10 '22 19:11 ericglau

We should also determine why the tests do not catch this.

ericglau avatar Nov 10 '22 19:11 ericglau

Yes, the issue came from handling the class hash in declare and not removing the hex conversion in register_class_hash (#257). The issue should be fixed in main from the #122 merge.

We should also determine why the tests do not catch this.

I agree^. I propose more granular tests

andrew-fleming avatar Nov 10 '22 23:11 andrew-fleming