Skip to content

Conversation

@noa-starkware
Copy link
Collaborator

@noa-starkware noa-starkware commented Oct 26, 2025

This change is Reviewable

Copy link
Collaborator

@remollemo remollemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remollemo reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @arad-starkware)


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

            amount: u256,
        ) {
            assert(amount > 0, Errors::AMOUNT_IS_ZERO);

why do we care if the user migrate zero amount? are we subsidizing the tx? if both tokens allow 0 transfer - welcome. if they don't - welcomer. why we need to add this code to our contract?
moreover, if we have a need to trigger a send to l1 (say we had too big a pile and only half of it was sent) then this is even a backdoor to do that.

Code quote:

 assert(amount > 0, Errors::AMOUNT_IS_ZERO);

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

pub mod Errors {

why not ALL_CAPS messages rahter than spongy half hearted error messages that are nishta here, nishta here?
you see them all caps from mile, you don't have to squint your eyes and try to guess where it is and what it means.

e.g. ZERO_AMOUNT is way clearer than 'Amount is suspected to be non-negative and non-posative at the same time'


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

    let res = usdc_migration_safe_dispatcher.swap_to_new(:amount);
    assert_panic_with_error(res, Erc20Error::INSUFFICIENT_BALANCE.describe());

note how much work added for a use case that is marginal at very very best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants