-
Notifications
You must be signed in to change notification settings - Fork 0
feat: impl constructor #7
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
Conversation
2f21419 to
1635bc8
Compare
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.
@remollemo reviewed 1 of 5 files at r1.
Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @arad-starkware)
packages/usdc_migration/src/usdc_migration.cairo line 14 at r1 (raw file):
usdc_e_token: ContractAddress, /// New USDC token address. usdc_token: ContractAddress,
i'd treat them (and name them) as new_token (native) and legacy_token
Suggestion:
/// The phased out token being swapped for the new one.
legacy_token: ContractAddress,
/// The new token swapping the legacy one.
new_token: ContractAddress,packages/usdc_migration/src/usdc_migration.cairo line 16 at r1 (raw file):
usdc_token: ContractAddress, /// Address in L1 that gets the USDC.e. owner_l1_address: ContractAddress,
- There is no L1 owner. it doesn't own a thing in this contract.
- The L1 address is the target fo the funds clearance. name it accordingly. e.g. -
l1_recipientif you insist - you can call itl1_fund_owner - if it's an L1 address the type should be EthAddress and not ContractAddress
Suggestion:
/// Ethereum address to which the legacy token is bridged.
l1_fund_owner: ContractAddress,packages/usdc_migration/src/usdc_migration.cairo line 32 at r1 (raw file):
usdc_token: ContractAddress, owner_l1_address: ContractAddress, owner_l2_address: ContractAddress,
we also need ther SG bridge address here.
1635bc8 to
4e75b66
Compare
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.
Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @arad-starkware and @remollemo)
packages/usdc_migration/src/usdc_migration.cairo line 14 at r1 (raw file):
Previously, remollemo wrote…
i'd treat them (and name them) as new_token (native) and legacy_token
Done.
packages/usdc_migration/src/usdc_migration.cairo line 16 at r1 (raw file):
Previously, remollemo wrote…
- There is no L1 owner. it doesn't own a thing in this contract.
- The L1 address is the target fo the funds clearance. name it accordingly. e.g. -
l1_recipientif you insist - you can call itl1_fund_owner- if it's an L1 address the type should be EthAddress and not ContractAddress
Done.
packages/usdc_migration/src/usdc_migration.cairo line 32 at r1 (raw file):
Previously, remollemo wrote…
we also need ther SG bridge address here.
Done.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @noa-starkware and @remollemo)
packages/usdc_migration/src/tests/test_usdc_migration.cairo line 30 at r2 (raw file):
); // Assert infinite approval to owner_l2_address for both USDC.e and USDC. let legacy_dispacther = IERC20Dispatcher { contract_address: cfg.legacy_token };
dispatcher
Code quote:
dispactherpackages/usdc_migration/src/tests/test_usdc_migration.cairo line 31 at r2 (raw file):
// Assert infinite approval to owner_l2_address for both USDC.e and USDC. let legacy_dispacther = IERC20Dispatcher { contract_address: cfg.legacy_token }; let native_dispacther = IERC20Dispatcher { contract_address: cfg.native_token };
same
Code quote:
dispactherThere 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.
Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @noa-starkware)
packages/usdc_migration/src/usdc_migration.cairo line 16 at r2 (raw file):
/// The new token swapping the legacy one. // TODO: Consider change to dispatcher. native_token: ContractAddress,
- don't change to dispatcher. IMO, but it can work that way too.
- native is too specific imo. wheres legacy vs. new is tighter with what really we do, and more generic.
Suggestion:
new_token: ContractAddress,packages/usdc_migration/src/usdc_migration.cairo line 48 at r2 (raw file):
let native_dispacther = IERC20Dispatcher { contract_address: native_token }; legacy_dispacther.approve(spender: owner_l2_address, amount: MAX_U256); native_dispacther.approve(spender: owner_l2_address, amount: MAX_U256);
inline it.
Suggestion:
IERC20Dispatcher { contract_address: legacy_token }.approve(spender: owner_l2_address, amount: MAX_U256);
IERC20Dispatcher { contract_address: native_token }.approve(spender: owner_l2_address, amount: MAX_U256);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.
Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @remollemo)
packages/usdc_migration/src/usdc_migration.cairo line 16 at r2 (raw file):
Previously, remollemo wrote…
- don't change to dispatcher. IMO, but it can work that way too.
- native is too specific imo. wheres legacy vs. new is tighter with what really we do, and more generic.
We only use it as dispatcher, so why not change to dispatcher here?
4e75b66 to
9b1b2d5
Compare
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.
Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @arad-starkware and @remollemo)
packages/usdc_migration/src/tests/test_usdc_migration.cairo line 30 at r2 (raw file):
Previously, arad-starkware wrote…
dispatcher
Done.
packages/usdc_migration/src/tests/test_usdc_migration.cairo line 31 at r2 (raw file):
Previously, arad-starkware wrote…
same
Done.
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.
@arad-starkware reviewed 2 of 5 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noa-starkware and @remollemo)
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noa-starkware and @remollemo)
605d50e to
491df52
Compare
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.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @noa-starkware and @remollemo)
packages/usdc_migration/src/usdc_migration.cairo line 20 at r4 (raw file):
owner_l2_address: ContractAddress, /// Token bridge address. starkgate_address: ContractAddress,
dispatcher as well?
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.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @noa-starkware and @remollemo)
packages/usdc_migration/src/usdc_migration.cairo line 20 at r4 (raw file):
Previously, arad-starkware wrote…
dispatcher as well?
You don't have the dispatcher here, I'll add it in a different PR, remove this
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.
Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @noa-starkware and @remollemo)
packages/usdc_migration/src/tests/test_utils.cairo line 19 at r4 (raw file):
use starknet::{ContractAddress, EthAddress}; pub const INITIAL_SUPPLY: u256 = 1000000000000000000000000000;
use pow? it's unclear what number this is...
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.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @arad-starkware and @noa-starkware)
packages/usdc_migration/src/usdc_migration.cairo line 16 at r2 (raw file):
Previously, noa-starkware wrote…
We only use it as dispatcher, so why not change to dispatcher here?
we can. i just don't like storing non-premitive types. but that is not a good reason not to.
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.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @arad-starkware)
packages/usdc_migration/src/usdc_migration.cairo line 20 at r4 (raw file):
Previously, arad-starkware wrote…
You don't have the dispatcher here, I'll add it in a different PR, remove this
Change it to dispatcher in your pr if you want to
packages/usdc_migration/src/tests/test_utils.cairo line 19 at r4 (raw file):
Previously, arad-starkware wrote…
use pow? it's unclear what number this is...
What number do we want here?
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.
@remollemo reviewed 1 of 5 files at r1, 1 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arad-starkware)
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noa-starkware)
packages/usdc_migration/src/usdc_migration.cairo line 20 at r4 (raw file):
Previously, noa-starkware wrote…
Change it to dispatcher in your pr if you want to
I will, but why add it here if you don't use it, and it will be overwritten?
packages/usdc_migration/src/tests/test_utils.cairo line 19 at r4 (raw file):
Previously, noa-starkware wrote…
What number do we want here?
we can make it the entire USDC.e supply, ~140M, so 140 * million * decimals or 14 * 10 million * decimals or just 14 * 10^12
but my point is i think it should be clearer what number this is (i.e. how many zeros)
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noa-starkware)

This change is