solana: allow pools to specify writable accounts #419
Conversation
chains/solana/contracts/programs/ccip-router/src/instructions/v1/token_admin_registry.rs
Outdated
Show resolved
Hide resolved
chains/solana/contracts/programs/ccip-router/src/instructions/v1/pools.rs
Outdated
Show resolved
Hide resolved
| expected_is_writable.append(&mut remaining_is_writable); | ||
| for (i, is_writable) in expected_is_writable.iter().enumerate() { | ||
| require!( | ||
| token_admin_registry_account.is_writable(i as u8) == *is_writable, |
There was a problem hiding this comment.
I'm not sure I understand this check
There was a problem hiding this comment.
i'll add some more comments - but it basically checks that the provided token pool account is (or is not) writable based on what's configured in the token admin registry (based on index)
There was a problem hiding this comment.
Nit: if you use require_eq! instead of require!, the logs will have more info in case the check fails.
|
|
||
| // TODO: Validate here that the lookup table has everything | ||
| // set writable indexes |
There was a problem hiding this comment.
What is this doing? Any of the accounts passed can be writable? Like the regular accounts and the additionals?
There was a problem hiding this comment.
correct, since these accounts are token pool specific, we need a way to know how to build the transaction offchain
this way provides an onchain record that admins can use to set which token pool accounts are writable
…v1/token_admin_registry.rs Co-authored-by: Jonghyeon Park <[email protected]>
aalu1418
force-pushed
the
solana/specify-writable-pool-accounts
branch
from
3a04b12 to
4eba5bd
Compare
|
|
||
| // TODO: Validate here that the lookup table has everything | ||
| // validate lookup table contains minimum required accounts if not zero address | ||
| if new_pool != Pubkey::zeroed() { |
There was a problem hiding this comment.
nit: If we use Pubkey::default() it returns the same thing as Pubkey::zeroed() and can make you remove the dependency on bytemuck::Zeroable here
| if new_pool != Pubkey::zeroed() { | |
| if new_pool != Pubkey::default() { |
| AddressLookupTable::deserialize(lookup_table_data) | ||
| .map_err(|_| CcipRouterError::InvalidInputsLookupTableAccounts)?; | ||
| require!( | ||
| lookup_table_account.addresses.len() >= 9, |
There was a problem hiding this comment.
Any chance you can link that 9 to a constant? It seems like that value is tied to logic in this file and in pools.rs, so it'd be nice if the compiler can check both places stay consistent in case we ever change one of them.
E.g., define a constant, use it here in the check, and use it in an explicit slice type in this line
Said constant could be taken outside of the versioned logic folders if we consider that part of our interface with TokenPools.
Also, if you use require_gte! instead of require!, the logs will print the actual length in case of error, which can help when troubleshooting failed tests for example
| expected_is_writable.append(&mut remaining_is_writable); | ||
| for (i, is_writable) in expected_is_writable.iter().enumerate() { | ||
| require!( | ||
| token_admin_registry_account.is_writable(i as u8) == *is_writable, |
There was a problem hiding this comment.
Nit: if you use require_eq! instead of require!, the logs will have more info in case the check fails.
| impl TokenAdminRegistry { | ||
| // set writable inserts bits from left to right | ||
| // index 0 is left-most bit | ||
| pub fn set_writable(&mut self, index: u8) { | ||
| match index < 128 { | ||
| true => { | ||
| self.writable_indexes[0] |= 1 << (127 - index); | ||
| } | ||
| false => { | ||
| self.writable_indexes[1] |= 1 << (255 - index); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn is_writable(&self, index: u8) -> bool { | ||
| match index < 128 { | ||
| true => self.writable_indexes[0] & 1 << (127 - index) != 0, | ||
| false => self.writable_indexes[1] & 1 << (255 - index) != 0, | ||
| } | ||
| } | ||
|
|
||
| pub fn reset_writable(&mut self) { | ||
| self.writable_indexes = [0, 0]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Up to you whether to leave this here for now or not, but in general I'd rather have functions defined in the versioned logic modules instead of here.
|
i will address these in a follow up PR! they seem pretty straight forward and I don't want any of my changes to block any refactors |
SetPool