Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: improve how depositor ATA interactions work #710

Merged
merged 6 commits into from
Nov 1, 2024
Merged

feat: improve how depositor ATA interactions work #710

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1310fd9
WIP
chrismaree Oct 31, 2024
0d64f17
test hook
chrismaree Oct 31, 2024
011ac23
test hook
chrismaree Oct 31, 2024
9391010
WIP
chrismaree Oct 31, 2024
fde744f
WIP
chrismaree Oct 31, 2024
196f60c
WIP
chrismaree Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions programs/svm-spoke/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ pub enum SvmError {
InvalidClaimInitializer,
#[msg("Seed must be 0 in production!")]
InvalidProductionSeed,
#[msg("Depositor Must be signer!")]
DepositorMustBeSigner,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is now redundant

#[msg("Invalid remaining accounts for ATA creation!")]
InvalidATACreationAccounts,
}
Expand Down
30 changes: 11 additions & 19 deletions programs/svm-spoke/src/instructions/deposit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,13 @@ use crate::{
output_token: Pubkey,
input_amount: u64,
output_amount: u64,
destination_chain_id: u64, // TODO: we can remove some of these instructions props
exclusive_relayer: Pubkey,
quote_timestamp: u32,
fill_deadline: u32,
exclusivity_deadline: u32,
message: Vec<u8>
destination_chain_id: u64,
)]
pub struct DepositV3<'info> {
#[account(mut)]
#[account(
mut,
constraint = signer.key() == depositor @ SvmError::DepositorMustBeSigner
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have this restriction in EVM where sender can provide any depositor address

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Reinis-FRP Reinis-FRP Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

)]
pub signer: Signer<'info>,
#[account(
mut,
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

this was explicit check in EVM implementation. why move it here in constraints?

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved it to match how we apply similar constraints on fill.rs

contracts/programs/svm-spoke/src/instructions/fill.rs

Line 27 in 5c1dc6e

constraint = !state.paused_fills @ CommonError::FillsArePaused

Copy link
Contributor

Choose a reason for hiding this comment

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

For fills EVM implementation has unpausedFills modifier, so we moved it to constraints so that implementation logic is as close as possible in SVM

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

)]
pub route: Account<'info, Route>,

#[account(
mut,
token::mint = mint,
token::authority = signer,
associated_token::mint = mint,
associated_token::authority = depositor,
token::token_program = token_program
Copy link
Contributor

Choose a reason for hiding this comment

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

change also token_program constraint to use associated_token

)]
pub user_token_account: InterfaceAccount<'info, TokenAccount>,
pub depositor_token_account: InterfaceAccount<'info, TokenAccount>,

#[account(
mut,
Expand All @@ -59,7 +58,6 @@ pub struct DepositV3<'info> {

#[account(
mint::token_program = token_program,
// IDL build fails when requiring `address = input_token` for mint, thus using a custom constraint.
constraint = mint.key() == input_token @ SvmError::InvalidMint
)]
pub mint: InterfaceAccount<'info, Mint>,
Expand All @@ -84,10 +82,6 @@ pub fn deposit_v3(
) -> Result<()> {
let state = &mut ctx.accounts.state;

if !ctx.accounts.route.enabled {
return err!(CommonError::DisabledRoute);
}

let current_time = get_current_time(state)?;

// TODO: if the deposit quote timestamp is bad it is possible to make this error with a subtraction
Expand All @@ -102,7 +96,7 @@ pub fn deposit_v3(
}

let transfer_accounts = TransferChecked {
from: ctx.accounts.user_token_account.to_account_info(),
from: ctx.accounts.depositor_token_account.to_account_info(),
mint: ctx.accounts.mint.to_account_info(),
to: ctx.accounts.vault.to_account_info(),
authority: ctx.accounts.signer.to_account_info(),
Expand Down Expand Up @@ -130,5 +124,3 @@ pub fn deposit_v3(

Ok(())
}

// TODO: do we need other flavours of deposit? like speed up deposit
4 changes: 2 additions & 2 deletions test/svm/SvmSpoke.Deposit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ describe("svm_spoke.deposit", () => {
state,
route,
signer: depositor.publicKey,
userTokenAccount: depositorTA,
depositorTokenAccount: depositorTA,
vault,
mint: inputToken,
tokenProgram: TOKEN_PROGRAM_ID,
Expand Down Expand Up @@ -368,7 +368,7 @@ describe("svm_spoke.deposit", () => {
state: fakeState,
route: fakeRoutePda,
signer: depositor.publicKey,
userTokenAccount: depositorTA,
depositorTokenAccount: depositorTA,
vault: fakeVault,
mint: inputToken,
tokenProgram: TOKEN_PROGRAM_ID,
Expand Down
Loading