-
Notifications
You must be signed in to change notification settings - Fork 353
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
Migrate UDC #919
Migrate UDC #919
Conversation
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.
Looking good andrew! Left some comments
src/presets/universal_deployer.cairo
Outdated
let mut from_zero: bool = true; | ||
|
||
if unique { | ||
_salt = pedersen(deployer.into(), salt); |
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.
Should we consider using poseidon since is much cheaper? Maybe a UDC_v2?
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.
Huge +1 to UDC_v2 with poseidon
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.
+1, but note this UDC is already v2
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 think we may need to be explicit in the name regarding the version. If people are using UDC already to predict addresses before deployment, getting confused about whether it uses pedersen or poseidon could be problematic, since the addresses would be different.
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.
Should we update to Poseidon then on this PR? And should we add documentation about it and versions? In the starknet docsite we are not for example mentioning which hash algorithm is used, or how to precompute the addresses, and I think that's worth to mention.
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.
Made this an issue: #950
Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>
assert_eq!(expected_addr, deployed_addr); | ||
|
||
// Check event | ||
let event = utils::pop_log::<UniversalDeployer::Event>(udc.contract_address).unwrap(); |
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.
Should we move this into a helper for consistency?
src/presets/universal_deployer.cairo
Outdated
let mut from_zero: bool = true; | ||
|
||
if unique { | ||
_salt = pedersen(deployer.into(), salt); |
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 think we may need to be explicit in the name regarding the version. If people are using UDC already to predict addresses before deployment, getting confused about whether it uses pedersen or poseidon could be problematic, since the addresses would be different.
src/presets/universal_deployer.cairo
Outdated
let mut from_zero: bool = true; | ||
|
||
if unique { | ||
_salt = pedersen(deployer.into(), salt); |
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.
Should we update to Poseidon then on this PR? And should we add documentation about it and versions? In the starknet docsite we are not for example mentioning which hash algorithm is used, or how to precompute the addresses, and I think that's worth to mention.
Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>
Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>
Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>
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.
looking good!
Co-authored-by: Martín Triay <martriay@gmail.com>
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 need to fix that one variable name and good to go
Co-authored-by: Martín Triay <martriay@gmail.com>
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.
Left a small suggestion for consistency, but lgtm!
Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>
Fixes #552.
PR Checklist