feat: migrate function implementation

[arad-starkware]

This change is 

[arad-starkware]

• feat: migrate function implementation #16 : 2 dependent PRs (#17 , #26 ) 👈 (View in Graphite)
• test: use snforge::Token #25
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[noa-starkware]

Reviewable status: 0 of 6 files reviewed, 10 unresolved discussions (waiting on @arad-starkware)

---

packages/usdc_migration/src/usdc_migration.cairo line 29 at r1 (raw file):

    #[event]
    #[derive(Drop, starknet::Event)]
    pub enum Event { //event variables

delete

Code quote:

//event variables

---

packages/usdc_migration/src/usdc_migration.cairo line 72 at r1 (raw file):

            };
            legacy_token_dispatcher
                .transfer_from(sender: caller_address, recipient: contract_address, :amount);

use checked transfer from utils
starkware-libs/starkware-starknet-utils#123

---

packages/usdc_migration/src/tests/test_utils.cairo line 72 at r1 (raw file):

pub(crate) fn new_user(cfg: USDCMigrationCfg, id: u8, amount: u256) -> ContractAddress {
    let user_address = generate_user_address(:id);
    let legacy_token_dispatcher = IERC20Dispatcher { contract_address: cfg.legacy_token };

Maybe we should work with snforge_std::Token

---

packages/usdc_migration/src/events.cairo line 5 at r1 (raw file):

    #[derive(Drop, starknet::Event, Debug, PartialEq)]
    pub struct USDCMigratedEvent {

Are we sure we need that event?

---

packages/usdc_migration/src/events.cairo line 7 at r1 (raw file):

    pub struct USDCMigratedEvent {
        pub amount: u256,
        pub user: ContractAddress,

Do we want key field?

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 55 at r1 (raw file):

    let amount = INITIAL_SUPPLY / 10;
    let user = new_user(:cfg, id: 0, :amount);
    supply_migration_contract_with_native(:cfg, :amount);

Can we include this in the deploy function? I assume its required by most of the tests

Code quote:

supply_migration_contract_with_native(:cfg, :amount);

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 123 at r1 (raw file):

    cheat_caller_address_once(contract_address: usdc_migration_contract, caller_address: user);
    usdc_migration_dispatcher.migrate(:amount);
}

I think we decided to have one test with safe dispatcher to catch all assertions. And not use should_panic. WDYT?

Code quote:

#[test]
#[should_panic(expected: "Insufficient USDC balance")]
fn test_migrate_insufficient_usdc_balance() {
    let cfg = deploy_usdc_migration();
    let amount = INITIAL_SUPPLY / 10;
    let user = new_user(:cfg, id: 0, :amount);
    let usdc_migration_contract = cfg.usdc_migration_contract;
    let usdc_migration_dispatcher = IUSDCMigrationDispatcher {
        contract_address: usdc_migration_contract,
    };

    // Migrate with insufficient USDC balance.
    cheat_caller_address_once(contract_address: usdc_migration_contract, caller_address: user);
    usdc_migration_dispatcher.migrate(:amount);
}

#[test]
#[should_panic(expected: 'ERC20: insufficient allowance')]
fn test_migrate_insufficient_allowance() {
    let cfg = deploy_usdc_migration();
    let amount = INITIAL_SUPPLY / 10;
    let user = new_user(:cfg, id: 0, :amount);
    supply_migration_contract_with_native(:cfg, :amount);
    let usdc_migration_contract = cfg.usdc_migration_contract;
    let usdc_migration_dispatcher = IUSDCMigrationDispatcher {
        contract_address: usdc_migration_contract,
    };
    let legacy_dispatcher = IERC20Dispatcher { contract_address: cfg.legacy_token };

    // Approve and migrate.
    cheat_caller_address_once(contract_address: cfg.legacy_token, caller_address: user);
    legacy_dispatcher.approve(spender: usdc_migration_contract, amount: amount / 2);
    cheat_caller_address_once(contract_address: usdc_migration_contract, caller_address: user);
    usdc_migration_dispatcher.migrate(:amount);
}

---

packages/usdc_migration/src/interface.cairo line 3 at r1 (raw file):

#[starknet::interface]
pub trait IUSDCMigration<T> {
    /// Migrate `amount` of USDC.e to USDC.

change to legacy and native

---

packages/usdc_migration/src/interface.cairo line 4 at r1 (raw file):

pub trait IUSDCMigration<T> {
    /// Migrate `amount` of USDC.e to USDC.
    /// Caller is expected to have approved the contract to spend `amount` of USDC.e.

"Precondition: ..."

---

packages/usdc_migration/src/interface.cairo line 5 at r1 (raw file):

    /// Migrate `amount` of USDC.e to USDC.
    /// Caller is expected to have approved the contract to spend `amount` of USDC.e.
    fn migrate(ref self: T, amount: u256);

rename

[arad-starkware]

Reviewable status: 0 of 6 files reviewed, 10 unresolved discussions (waiting on @arad-starkware and @noa-starkware)

---

packages/usdc_migration/src/events.cairo line 5 at r1 (raw file):

Previously, noa-starkware wrote…

Are we sure we need that event?

I think yes

---

packages/usdc_migration/src/events.cairo line 7 at r1 (raw file):

Previously, noa-starkware wrote…

Do we want key field?

yes

---

packages/usdc_migration/src/interface.cairo line 3 at r1 (raw file):

Previously, noa-starkware wrote…

change to legacy and native

I see native was changed to "new"

---

packages/usdc_migration/src/interface.cairo line 4 at r1 (raw file):

Previously, noa-starkware wrote…

"Precondition: ..."

Done.

---

packages/usdc_migration/src/interface.cairo line 5 at r1 (raw file):

Previously, noa-starkware wrote…

rename

how about this?

---

packages/usdc_migration/src/usdc_migration.cairo line 29 at r1 (raw file):

Previously, noa-starkware wrote…

delete

Done.

---

packages/usdc_migration/src/usdc_migration.cairo line 72 at r1 (raw file):

Previously, noa-starkware wrote…

use checked transfer from utils
starkware-libs/starkware-starknet-utils#123

I added a TODO,

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 55 at r1 (raw file):

Previously, noa-starkware wrote…

Can we include this in the deploy function? I assume its required by most of the tests

I added a new function as a wrapper for both (as some tests will need balance 0).

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 123 at r1 (raw file):

Previously, noa-starkware wrote…

I think we decided to have one test with safe dispatcher to catch all assertions. And not use should_panic. WDYT?

I wasn't aware, I agree

---

packages/usdc_migration/src/tests/test_utils.cairo line 72 at r1 (raw file):

Previously, noa-starkware wrote…

Maybe we should work with snforge_std::Token

what's the benefit?

[noa-starkware]

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @arad-starkware)

---

packages/usdc_migration/src/tests/test_utils.cairo line 72 at r1 (raw file):

Previously, arad-starkware wrote…

what's the benefit?

We dont need to deploy manually, and work with dispatcher and transfer or other ABI functions, you can take a look at staking tests.

[noa-starkware]

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @arad-starkware)

---

packages/usdc_migration/src/usdc_migration.cairo line 72 at r1 (raw file):

Previously, arad-starkware wrote…

I added a TODO,

The PR is merged

[noa-starkware]

Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @arad-starkware)

---

packages/usdc_migration/src/tests/test_utils.cairo line 77 at r3 (raw file):

        owner_l2_address: OWNER_ADDRESS(),
        starkgate_address: STARKGATE_ADDRESS(),
        initial_contract_supply: INITIAL_SUPPLY / 2,

Why did you add the initial_contract_supply as part of the cfg? and why did you choose this value?

---

packages/usdc_migration/src/tests/test_utils.cairo line 81 at r3 (raw file):

}

pub(crate) fn new_user(cfg: USDCMigrationCfg, id: u8, amount: u256) -> ContractAddress {

use better name. maybe legacy_supply

Code quote:

amount

---

packages/usdc_migration/src/tests/test_utils.cairo line 91 at r3 (raw file):

}

fn generate_user_address(id: u8) -> ContractAddress {

Suggestion:

_generate_user_address

---

packages/usdc_migration/src/tests/test_utils.cairo line 95 at r3 (raw file):

}

pub(crate) fn supply_migration_contract_with_new_token(cfg: USDCMigrationCfg, amount: u256) {

Suggestion:

initial_supply_migration_contract

---

packages/usdc_migration/src/tests/test_utils.cairo line 100 at r3 (raw file):

        contract_address: cfg.new_token, caller_address: cfg.owner_l2_address,
    );
    new_dispatcher.transfer(recipient: cfg.usdc_migration_contract, :amount);

again, maybe better to work with Token. WDYT?

[noa-starkware]

Reviewable status: 0 of 8 files reviewed, 7 unresolved discussions (waiting on @arad-starkware and @remollemo)

---

packages/usdc_migration/src/usdc_migration.cairo line 86 at r4 (raw file):

Previously, arad-starkware wrote…

I'd like to keep the error, do we need to think about re-entrancy if so?

We should rely on the error from checked_transfer_from. Otherwise there is no point in calling it

---

packages/usdc_migration/src/tests/test_utils.cairo line 77 at r3 (raw file):

Previously, arad-starkware wrote…

I don't understand why not to use some fraction of INITIAL_SUPPLY, it has to be less than that, why not bind the value to it?
If we get an actual number from finance / product, we can change to that, but why are you against using INITIAL_SUPPLY?

I also don't understand what's wrong with having it in the cfg, but I don't mind that much...

You said the value is meaningless, so I dont see a reason to use some other (meaningful) constant

[noa-starkware]

Reviewable status: 0 of 8 files reviewed, 9 unresolved discussions (waiting on @arad-starkware and @remollemo)

---

packages/usdc_migration/src/usdc_migration.cairo line 114 at r5 (raw file):

        fn _swap(
            ref self: ContractState,
            from_token_dispatcher: IERC20Dispatcher,

Suggestion:

from_token

---

packages/usdc_migration/src/usdc_migration.cairo line 115 at r5 (raw file):

            ref self: ContractState,
            from_token_dispatcher: IERC20Dispatcher,
            to_token_dispatcher: IERC20Dispatcher,

Suggestion:

to_token

[noa-starkware]

Reviewable status: 0 of 8 files reviewed, 10 unresolved discussions (waiting on @arad-starkware and @remollemo)

---

packages/usdc_migration/src/usdc_migration.cairo line 122 at r5 (raw file):

                amount <= to_token_dispatcher.balance_of(account: contract_address),
                "{}",
                USDCMigrationError::INSUFFICIENT_USDC_BALANCE,

No point in this error before calling checked_transfer_from. It already has this check+error

[noa-starkware]

Reviewable status: 0 of 8 files reviewed, 11 unresolved discussions (waiting on @arad-starkware and @remollemo)

---

packages/usdc_migration/src/usdc_migration.cairo line 136 at r5 (raw file):

                    USDCMigrated {
                        user: caller_address,
                        to_token: to_token_dispatcher.contract_address,

add from token field?

[noa-starkware]

Reviewable status: 0 of 8 files reviewed, 11 unresolved discussions (waiting on @arad-starkware and @remollemo)

---

packages/usdc_migration/src/usdc_migration.cairo line 122 at r5 (raw file):

Previously, noa-starkware wrote…

No point in this error before calling checked_transfer_from. It already has this check+error

sorry I meant checked_transfer

[noa-starkware]

Reviewable status: 0 of 8 files reviewed, 12 unresolved discussions (waiting on @arad-starkware and @remollemo)

---

packages/usdc_migration/src/errors.cairo line 11 at r5 (raw file):

    fn describe(self: @USDCMigrationError) -> ByteArray {
        match self {
            USDCMigrationError::INSUFFICIENT_USDC_BALANCE => "Insufficient USDC balance",

legacy? new? both?

Code quote:

USDC

[arad-starkware]

Reviewable status: 0 of 8 files reviewed, 9 unresolved discussions (waiting on @arad-starkware, @noa-starkware, and @remollemo)

---

packages/usdc_migration/src/errors.cairo line 11 at r5 (raw file):

Previously, noa-starkware wrote…

legacy? new? both?

no longer relevant

---

packages/usdc_migration/src/usdc_migration.cairo line 86 at r4 (raw file):

Previously, noa-starkware wrote…

We should rely on the error from checked_transfer_from. Otherwise there is no point in calling it

ok, note this changes the order of the errors (allowance will be checked before the contract's balance). not that i mind, just FYI

---

packages/usdc_migration/src/usdc_migration.cairo line 122 at r5 (raw file):

Previously, noa-starkware wrote…

sorry I meant checked_transfer

Done.

---

packages/usdc_migration/src/usdc_migration.cairo line 136 at r5 (raw file):

Previously, noa-starkware wrote…

add from token field?

I thought about this, but it doesn't really add any new information, do you think it's better to include it? (I could go either way)

[remollemo]

@remollemo reviewed 2 of 5 files at r4, 2 of 5 files at r5, 1 of 3 files at r6, all commit messages.
Reviewable status: 5 of 8 files reviewed, 7 unresolved discussions (waiting on @arad-starkware and @noa-starkware)

---

packages/usdc_migration/src/usdc_migration.cairo line 86 at r4 (raw file):

Previously, arad-starkware wrote…

It's a good idea, but I'd like to do the other direction in a different PR if that's alright. I'll lay the groundwork here but the new function and tests will come in a later PR.

🤷‍♂️
reminds me of a very funny true story.

---

packages/usdc_migration/src/usdc_migration.cairo line 86 at r4 (raw file):

Previously, arad-starkware wrote…

ok, note this changes the order of the errors (allowance will be checked before the contract's balance). not that i mind, just FYI

"We should rely on the error from checked_transfer_from. Otherwise there is no point in calling it"
+1

---

packages/usdc_migration/src/usdc_migration.cairo line 92 at r4 (raw file):

Previously, arad-starkware wrote…

I'd like to think we do

i would like to see better error messages in this repo too
where can i find them?

---

packages/usdc_migration/src/usdc_migration.cairo line 136 at r5 (raw file):

Previously, arad-starkware wrote…

I thought about this, but it doesn't really add any new information, do you think it's better to include it? (I could go either way)

it's better to include it, as it looks kinda weird.
(but no drama)

---

a discussion (no related file):
Even though the scope is specific to migration of USDC - the impl should be token agnostic.
today it's usdc, tomorrow it's usdt. there is nothing specific to USDC in the processing.
we perform 1-1 swap between two tokens (we do reckon it's the new and legacy, ok, even though we don't even check that they are identical ) and we send the legacy one over.

we need not USDC ourselves anywhere inside (i'd even change the contrct and file name).

---

packages/usdc_migration/src/interface.cairo line 4 at r6 (raw file):

pub trait IUSDCMigration<T> {
    /// Swaps `amount` of legacy token for new token.
    /// Precondition: Caller has approved the contract to spend `amount` of legacy token.

2 trumps 11.

Suggestion:

/// Precondition: Sufficient allowance.

[arad-starkware]

Reviewable status: 2 of 8 files reviewed, 6 unresolved discussions (waiting on @noa-starkware and @remollemo)

---

a discussion (no related file):

Previously, remollemo wrote…

Even though the scope is specific to migration of USDC - the impl should be token agnostic.
today it's usdc, tomorrow it's usdt. there is nothing specific to USDC in the processing.
we perform 1-1 swap between two tokens (we do reckon it's the new and legacy, ok, even though we don't even check that they are identical ) and we send the legacy one over.

we need not USDC ourselves anywhere inside (i'd even change the contrct and file name).

The thing that makes it somewhat specific to USDC in my eyes is that the design we agreed upon was for this specific migration, and I can't assume all token migrations will follow this design. I agree that within the implementation there's no need to bind ourselves to USDC, but I think changing the contract name at this point is a bit excessive.

---

packages/usdc_migration/src/usdc_migration.cairo line 86 at r4 (raw file):

Previously, remollemo wrote…

"We should rely on the error from checked_transfer_from. Otherwise there is no point in calling it"
+1

Done.

---

packages/usdc_migration/src/usdc_migration.cairo line 92 at r4 (raw file):

Previously, remollemo wrote…

i would like to see better error messages in this repo too
where can i find them?

I removed the only error in the contract, so by the null hypothesis, all the errors in the contract are very good.

---

packages/usdc_migration/src/usdc_migration.cairo line 136 at r5 (raw file):

Previously, remollemo wrote…

it's better to include it, as it looks kinda weird.
(but no drama)

Done

[remollemo]

@remollemo reviewed 2 of 4 files at r7.
Reviewable status: 4 of 8 files reviewed, 7 unresolved discussions (waiting on @noa-starkware)

---

a discussion (no related file):

Previously, arad-starkware wrote…

The thing that makes it somewhat specific to USDC in my eyes is that the design we agreed upon was for this specific migration, and I can't assume all token migrations will follow this design. I agree that within the implementation there's no need to bind ourselves to USDC, but I think changing the contract name at this point is a bit excessive.

nothing specific at all (other than the circumstances)
any SG bridge token replacement with an external bridge we'd do the same.

---

packages/usdc_migration/src/usdc_migration.cairo line 118 at r7 (raw file):

        ) {
            let contract_address = get_contract_address();
            let caller_address = get_caller_address();

Suggestion:

user 

---

packages/usdc_migration/src/usdc_migration.cairo line 130 at r7 (raw file):

                        user: caller_address,
                        from_token: from_token.contract_address,
                        to_token: to_token.contract_address,

@noa-starkware - see?

and we end up with this **** ** **** ** ****!

Code quote:

                        from_token: from_token.contract_address,
                        to_token: to_token.contract_address,

[remollemo]

@remollemo reviewed 1 of 5 files at r5, 1 of 3 files at r6, 2 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @arad-starkware and @noa-starkware)

[remollemo]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @arad-starkware and @noa-starkware)

---

packages/usdc_migration/src/usdc_migration.cairo line 92 at r4 (raw file):

Previously, arad-starkware wrote…

I removed the only error in the contract, so by the null hypothesis, all the errors in the contract are very good.

:LMAO:
errors are indeed good. (at least for ones who love errors). The error messages are awful.

[remollemo]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @arad-starkware and @noa-starkware)

[noa-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arad-starkware and @remollemo)

---

packages/usdc_migration/src/tests/test_utils.cairo line 104 at r7 (raw file):

}

pub(crate) fn new_user(cfg: USDCMigrationCfg, id: u8, legacy_supply: u256) -> ContractAddress {

I dont like it gets the id

Code quote:

id: u8

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 192 at r7 (raw file):

    let legacy_dispatcher = IERC20Dispatcher { contract_address: legacy_token_address };

    // Insufficient allowance.

What about sufficient allowance but insufficient balance of the caller?

---

packages/usdc_migration/src/usdc_migration.cairo line 118 at r7 (raw file):

        ) {
            let contract_address = get_contract_address();
            let caller_address = get_caller_address();

Agree

[arad-starkware]

Reviewable status: 6 of 8 files reviewed, 4 unresolved discussions (waiting on @noa-starkware and @remollemo)

---

a discussion (no related file):

Previously, remollemo wrote…

nothing specific at all (other than the circumstances)
any SG bridge token replacement with an external bridge we'd do the same.

@noa-starkware WDYT?

---

packages/usdc_migration/src/usdc_migration.cairo line 118 at r7 (raw file):

Previously, noa-starkware wrote…

Agree

Done.

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 192 at r7 (raw file):

Previously, noa-starkware wrote…

What about sufficient allowance but insufficient balance of the caller?

Done.

---

packages/usdc_migration/src/tests/test_utils.cairo line 104 at r7 (raw file):

Previously, noa-starkware wrote…

I dont like it gets the id

in staking we take advantage of system to keep track of ids behind the scenes, but I thought it would be overkill to implement a struct and pass it around just for this. do you think it's better to create a SystemState here? at the end of the day we'd be replacing one param with another... the only difference would be the new param will keep track of the ids on its own. (I'm against)

[noa-starkware]

Reviewable status: 6 of 8 files reviewed, 4 unresolved discussions (waiting on @arad-starkware and @remollemo)

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 203 at r8 (raw file):

    supply_contract(target: user, token: cfg.legacy_token, :amount);
    cheat_caller_address_once(contract_address: legacy_token_address, caller_address: user);
    legacy_dispatcher.approve(spender: usdc_migration_contract, amount: amount / 2);

How does it work? This approve overrides the previous one?

[noa-starkware]

Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @arad-starkware and @remollemo)

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 208 at r8 (raw file):

    assert_panic_with_error(res, Erc20Error::INSUFFICIENT_ALLOWANCE.describe());

    // Insufficient balance.

Suggestion:

Insufficient contract balance

[arad-starkware]

Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @noa-starkware and @remollemo)

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 203 at r8 (raw file):

Previously, noa-starkware wrote…

How does it work? This approve overrides the previous one?

yes

---

packages/usdc_migration/src/tests/test_usdc_migration.cairo line 208 at r8 (raw file):

    assert_panic_with_error(res, Erc20Error::INSUFFICIENT_ALLOWANCE.describe());

    // Insufficient balance.

Done.

[noa-starkware]

Reviewable status: 6 of 8 files reviewed, 3 unresolved discussions (waiting on @arad-starkware and @remollemo)