feat: improve how depositor ATA interactions work #710
Conversation
| token::mint = mint, | ||
| token::authority = signer, | ||
| associated_token::mint = mint, | ||
| associated_token::authority = depositor, | ||
| token::token_program = token_program |
There was a problem hiding this comment.
change also token_program constraint to use associated_token
| @@ -37,17 +35,18 @@ pub struct DepositV3<'info> { | |||
|
|
|||
| #[account( | |||
| seeds = [b"route", input_token.as_ref(), state.key().as_ref(), destination_chain_id.to_le_bytes().as_ref()], | |||
| bump | |||
| bump, | |||
| constraint = route.enabled @ CommonError::DisabledRoute | |||
There was a problem hiding this comment.
this was explicit check in EVM implementation. why move it here in constraints?
There was a problem hiding this comment.
I moved it to match how we apply similar constraints on fill.rs
There was a problem hiding this comment.
For fills EVM implementation has unpausedFills modifier, so we moved it to constraints so that implementation logic is as close as possible in SVM
There was a problem hiding this comment.
ye, I recall this. that still feels funny to me though. Would we not rather be consistent between rust implementation? the EVM logic feels inconsistent between the modifier use vs inline checks anyways.
| #[account(mut)] | ||
| #[account( | ||
| mut, | ||
| constraint = signer.key() == depositor @ SvmError::DepositorMustBeSigner |
There was a problem hiding this comment.
We don't have this restriction in EVM where sender can provide any depositor address
There was a problem hiding this comment.
you're right, we dont really care about this as long as the authority is the depositor. previously we checked the authority is the signer, but now we check it is the depositor, which is functionally different. the goal I was trying to achieve is that deposit will work for a multi-sig & correct ATA support, where the depositor is the owner of the ATA. I think how it's structured, without this constraint, will support multisigs where the authority is not the signer.
There was a problem hiding this comment.
yes, we don't really care that depositor is signer. and in case of multisig the authority account will be owned by the token program. we don't need to replicate any authorsation checks here as they will be performed in CPI transfer
programs/svm-spoke/src/error.rs
Outdated
| @@ -71,6 +71,8 @@ pub enum SvmError { | |||
| InvalidClaimInitializer, | |||
| #[msg("Seed must be 0 in production!")] | |||
| InvalidProductionSeed, | |||
| #[msg("Depositor Must be signer!")] | |||
| DepositorMustBeSigner, | |||
Signed-off-by: chrismaree christopher.maree@gmail.com