solana: refactor token accounts validation with Anchor context#756
solana: refactor token accounts validation with Anchor context#756jadepark-dev merged 7 commits intomainfrom
Conversation
toblich
left a comment
toblich
left a comment
There was a problem hiding this comment.
Looks great! Some small comments
| )] | ||
| pub user_token_account: InterfaceAccount<'info, TokenAccount>, | ||
|
|
||
| // TODO: determine if this can be zero key for optional billing config? |
There was a problem hiding this comment.
I think billing is not optional, and this TODO can be removed.
Basically, from the user's PoV, they are required to always pass in the billing accounts. Then, Fee Quoter can handle the case where the account is not initialized (i.e. there's no special billing configs for that case), but the user can't be the one to choose whether they pass it in or not.
| // Instead of manually checking each account (ownership, PDA derivation, constraints), | ||
| // we're using Anchor's `try_accounts` to perform these validations based on the | ||
| // constraints defined in the `TokenAccountsValidationContext` account context struct | ||
| let program_id = crate::id(); |
There was a problem hiding this comment.
For what it's worth, I spent a bit of time looking into Anchor's internals for this, and it seems like the try_accounts in this case will just ignore the program id set here (which is ok), so I'll ask you to please do the following:
- Run the tests setting here a random pubkey as well, and confirm that tests should pass just fine
- Then add a comment clarifying that this value is not used
My original concern was that, as we change program addresses from env to env, I'm not 100% sure that when we're compiling the offramp for a particular env the common crate will correctly have the router address for that same env. That's why I checked the code, and if you can just confirm empirically that it doesn't really matter and then document it, then this LGMT 👍
There was a problem hiding this comment.
Thanks for looking into it! Yes, it’s not being used at all—every PDA derivation in the TokenAccountsValidationContext has explicit program IDs. I’m opting to set it to a default Pubkey so that if we ever modify the context to rely on the context’s program ID, the tests will fail and alert us to the change.
2da1017 (#756)
PabloMansanet
left a comment
PabloMansanet
left a comment
There was a problem hiding this comment.
Looks great, pending addressing Tobi's concerns. Good work!
| let (user_token_account, remaining_accounts) = accounts.split_first().unwrap(); | ||
| let (token_billing_config, remaining_accounts) = remaining_accounts.split_first().unwrap(); | ||
| let (pool_chain_config, remaining_accounts) = remaining_accounts.split_first().unwrap(); | ||
| let user_token_account = next_account_info(&mut accounts_iter)?; |
There was a problem hiding this comment.
Very minor nit, but those accounts could be retrieved from the iterator a bit later, after the validations. That way it's a bit more clear when they're needed.
There was a problem hiding this comment.
Make sense, thanks! modified in 51cd88a (#756)
|
3 participants
This PR refactors the token account validation logic in CCIP Solana programs by leveraging Anchor's built-in account validation system.
Primary Changes
TokenAccountsValidationContextaccount struct in both offramp and router programsOthers
Removed redundant error codes (
InvalidInputsConfigAccounts)Created
AnchorErrorenum in common utils to standardize error string handling in testsUpdated tests to reference the new standardized error codes
Simplified account iteration with
next_account_infoinstead of manual splittingDependency: [Solana][NONEVM-1452] Multiple deduplications / refactor #750