-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to cairo v0.9.0 #364
Conversation
* add releasing.md * clarify branch role
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew! Long PR 😅
Great work, left some comments.
execution_info = await signer.send_transactions( | ||
admin, | ||
[ | ||
(proxy.contract_address, 'name', []), | ||
(proxy.contract_address, 'symbol', []), | ||
(proxy.contract_address, 'decimals', []), | ||
(proxy.contract_address, 'totalSupply', []) | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhhh so much better!
# fetch values | ||
execution_info = await signer.send_transactions( | ||
admin, | ||
[ | ||
(proxy.contract_address, 'balanceOf', [admin.contract_address]), | ||
(proxy.contract_address, 'balanceOf', [USER]), | ||
(proxy.contract_address, 'totalSupply', []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess this also speeds up testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to compare! I'll confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well it wasn't expected since the focus is not performance
tests/upgrades/test_Proxy.py
Outdated
# check initial admin | ||
execution_info = await signer.send_transaction( | ||
admin, proxy.contract_address, 'getAdmin', [] | ||
) | ||
assert execution_info.result.response == [admin.contract_address] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this necessary? it looks like we're testing the fixture instead of the library behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it's not necessary, more of a sanity check. We can remove it
@@ -15,7 +15,7 @@ passenv = | |||
HOME | |||
PYTHONPATH | |||
deps = | |||
cairo-lang==0.8.2.1 | |||
cairo-lang==0.9.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably release + pin nile too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
Co-authored-by: Martín Triay <martriay@gmail.com>
Co-authored-by: Martín Triay <martriay@gmail.com>
Fixes #360, #357.
Thiis PR updates the cairo-lang version in tox to
cairo-lang==0.9.0
. Further, this PR refactors all of the test modules in order to conform to cairo-lang's v0.9.0 release. The bulk of this PR's proposed changes include:contract_def
forcontract_class
in theStarknetContract
objectThis PR also proposes to refactor the proxy pattern by:
declaring
implementation contracts instead of deploying them like normal contractslibrary_call
andlibrary_call_l1_handler
instead ofdelegate_call
anddelegate_l1_handler
_hash
to storage and methods e.g.get_implementation_hash
instead ofget_implementation
Finally, this PR also proposes to add the
AdminChanged
event to the Proxy library.PR Checklist