solana: support any extra args on ccip_send #488
Conversation
aalu1418
commented
Jan 22, 2025
•
aalu1418
commented
- extra arg handling
- address validation
- test case fixes
aalu1418
force-pushed
the
solana/any-extra-args
branch
from
2f8cb00 to
c2718d1
Compare
aalu1418
requested review from
agusaldasoro,
toblich,
PabloMansanet and
jadepark-dev
| @@ -17,10 +17,9 @@ Collapsed Router + OnRamp + OffRamp Contracts Implementation for CCIP in SVM. | |||
|
|
|||
There was a problem hiding this comment.
Is this readme still valid? I remember we used to write all the TODOs here, but I don't know if that's still the case
There was a problem hiding this comment.
not sure - but i can go through and remove things since it's probably out of date
| pub fn default_config(cfg: &DestChainConfig) -> EVMExtraArgsV2 { | ||
| EVMExtraArgsV2 { | ||
| gas_limit: cfg.default_tx_gas_limit as u128, | ||
| ..Default::default() |
There was a problem hiding this comment.
What does this line do? Doesn't the DestChainConfig have the default value for OOO as well?
There was a problem hiding this comment.
not quite, the DestChain config has EnforceOutOfOrder to make sure that the user sets out of order - otherwise, we don't validate
and on the EVM side, if EnforceOutOfOrder = true then it requires the user to explicitly set it rather than use a default
|
|
||
| #[derive(Clone, AnchorSerialize, AnchorDeserialize, Default)] | ||
| pub struct SVMExtraArgsV1 { | ||
| pub compute_units: u32, |
There was a problem hiding this comment.
Are we doing something with this value? Or is this just for the offramp to know if they may need to increase the compute units?
There was a problem hiding this comment.
correct, this would be for the offchain side
There was a problem hiding this comment.
so we need it here for the hashing?
There was a problem hiding this comment.
this would be for sending from SVM -> SVM, but all extraArgs should be encoded as bytes during hashing
| pub struct SVMExtraArgsV1 { | ||
| pub compute_units: u32, | ||
| pub account_is_writable_bitmap: u64, | ||
| pub allow_out_of_order_execution: bool, |
There was a problem hiding this comment.
I think that this field can be removed? As it always will be out of order
There was a problem hiding this comment.
this aligns with the SVM ExtraArgs that are defined in the solidity contracts since this is just for the ccipSend side
(also see previous comment about requiring users to explicitly allow ooo processing even if it's required)
| pub account_is_writable_bitmap: u64, | ||
| pub allow_out_of_order_execution: bool, | ||
| pub token_receiver: [u8; 32], | ||
| pub accounts: Vec<[u8; 32]>, |
There was a problem hiding this comment.
Is this correct? Shouldn't this be part of the transaction in additional accounts?
There was a problem hiding this comment.
i should add a comment that these ExtraArgs are only for SVM -> XXX on the onramp (it has no connection to the offramp)
so the extra accounts would be passed in explicitly for the remote SVM chain
chains/solana/contracts/programs/ccip-router/src/instructions/v1/fee_quoter.rs
Outdated
Show resolved
Hide resolved
chains/solana/contracts/programs/ccip-router/src/instructions/v1/onramp.rs
Outdated
Show resolved
Hide resolved
| match u32::from_be_bytes(dest_config.chain_family_selector) { | ||
| CHAIN_FAMILY_SELECTOR_EVM => parse_and_validate_evm_extra_args(dest_config, tag, &mut data), | ||
| CHAIN_FAMILY_SELECTOR_SVM => { | ||
| parse_and_validate_svm_extra_args(dest_config, tag, &mut data, message_contains_tokens) |
There was a problem hiding this comment.
There was a problem hiding this comment.
the switch case is defined twice, yes
one function is for parsing/validating extra args, the other is for validating the receiver address
| @@ -306,13 +310,21 @@ fn token_transfer( | |||
| // Helpers // | |||
| ///////////// | |||
|
|
|||
| // ProcessedExtraArgs contains serialized extra args and unpacks commonly used parameters | |||
| #[derive(Debug)] | |||
| pub struct ProcessedExtraArgs { | |||
There was a problem hiding this comment.
Is this only for EVM should it declare it in the type?
There was a problem hiding this comment.
no it's chain agnostic - it has the serialized bytes, and two parameters that are checked for all chains (gas_limit + ooo)
|
