Conversation
… add token burning in migrationIdentity()
zkr99
left a comment
zkr99
left a comment
There was a problem hiding this comment.
Solid work on the migration. Field carry-over is comprehensive, the Approve+Burn delegation pattern is clean, and tests verify the full lifecycle (old IdentityState closed, old token burned, old mint closed). Failure-case test for empty-old-identity is a nice touch.
One blocker plus a few cleanup nits inline.
Blocker: MigradeIdentity typo in the struct + Context references. Anchor IDL surface, SDK consumers will see it. Quick rename to MigrateIdentity.
Other items (cleanup, design observations) flagged inline. None block the merge once the typo is fixed.
| pub token_account: InterfaceAccount<'info, TokenAccount>, | ||
| } | ||
| #[derive(Accounts)] | ||
| pub struct MigradeIdentity<'info> { |
There was a problem hiding this comment.
Typo blocker: MigradeIdentity -> MigrateIdentity here and in the Context<MigradeIdentity> at L261. The function name migrate_identity is correct - just the struct/Context references. This name goes into the Anchor IDL so any SDK consumer will see the typo.
| let rent = Rent::get()?; | ||
| let lamports = rent.minimum_balance(space); | ||
|
|
||
| //msg!("create_account"); |
There was a problem hiding this comment.
Debug //msg!(...) comments scattered through this body (L277, L293, L300, L309, L402) and in mint_anchor (L124, L140, L147, L156, L167, L181, L196). Worth cleaning up before merge - either delete or uncomment for production logging.
| drop(config_data); | ||
|
|
||
| // Transfer verification fee from user to protocol treasury | ||
| if verification_fee > 0 { |
There was a problem hiding this comment.
This charges the standard verification_fee on migration. Migration isn't a verification though, so semantically the fee is a bit off. Options: zero it out for migrations, add a separate migration_fee field on ProtocolConfig, or leave with a comment explaining the choice. Not blocking for hackathon scope.
| bump, | ||
| )] | ||
| pub identity_state: Box<Account<'info, IdentityState>>, | ||
| /// CHECK: TODO |
There was a problem hiding this comment.
/// CHECK: TODO here and at L890. Replace with meaningful descriptions of what these accounts are and what validates them (seeds + bump in the account constraint already validate the addresses).
| pub treasury: UncheckedAccount<'info>, | ||
|
|
||
| // below is for migration | ||
| /// CHECK: cannot check this wallet |
There was a problem hiding this comment.
This comment is misleading - wallet_old IS implicitly validated, just not directly. The seeds constraints on identity_state_old, mint_old, and token_account_old (which all derive PDAs from wallet_old.key()) reject any tx where wallet_old doesn't match. Suggested: /// CHECK: validated via seeds constraints on identity_state_old/mint_old/token_account_old.
| #[account( | ||
| mut, | ||
| seeds = [b"identity", wallet_old.key().as_ref()], | ||
| bump, close = user |
There was a problem hiding this comment.
close = user sends the old IdentityState's rent back to the new wallet. The old wallet originally paid for it, so close = wallet_old might match user expectation better. Either is defensible (the old wallet voluntarily gives up the identity), just calling out the choice so it's intentional.
2 participants
add
authorize_new_wallet()andmigrate_identity()add closing old identityPda after migration
use
mintAnchor()to makemigrationIdentiy()add failure test: migrateIdentity from user1 with empty Old Identity
add token delegation in
authorizeNewWallet()add token burning in
migrationIdentity()add closing old mint account during identity migration