feat: add determinstic deposit hash & new exclusivity logic #759
Conversation
chrismaree
changed the title
chrismaree
changed the title
| state.number_of_deposits += 1; // Increment number of deposits | ||
| let mut applied_deposit_id = deposit_id; | ||
| // If the passed in deposit_id is all zeros, then we use the state's number of deposits as deposit_id. | ||
| if deposit_id.iter().all(|&x| x == 0) { |
There was a problem hiding this comment.
Nit: maybe more concise to do if deposit_id == ZERO_DEPOSIT_ID
where
const ZERO_DEPOSIT_ID: [u8; 32] = [0u8; 32];
| output_amount, | ||
| destination_chain_id, | ||
| exclusive_relayer, | ||
| [0u8; 32], // deposit_id of informs internal function to use state.number_of_deposits as id. |
There was a problem hiding this comment.
Nit: we could store this in constants as it's reused multiple times
| // Define a dummy context struct so we can export this as a view function in lib. | ||
| #[derive(Accounts)] | ||
| pub struct Null {} | ||
| pub fn get_unsafe_deposit_id(msg_sender: Pubkey, depositor: Pubkey, deposit_nonce: u64) -> [u8; 32] { |
There was a problem hiding this comment.
Do you think it makes sense to expose this? Are we planning to use it off-chain, or should we implement a similar function in TS for efficiency?
There was a problem hiding this comment.
yes, I think having it exposed is useful. it's done in solidity and it's good to not force users to have to do this in js. for example say you wanted to do the deposit with etherscan for example. i don't think there is any downside in having it public. do you agree?
| if deposit_id.iter().all(|&x| x == 0) { | ||
| state.number_of_deposits += 1; | ||
| let mut deposit_id_bytes = [0u8; 32]; | ||
| deposit_id_bytes[..4].copy_from_slice(&state.number_of_deposits.to_le_bytes()); | ||
| applied_deposit_id = deposit_id_bytes; | ||
| } |
There was a problem hiding this comment.
| if deposit_id.iter().all(|&x| x == 0) { | |
| state.number_of_deposits += 1; | |
| let mut deposit_id_bytes = [0u8; 32]; | |
| deposit_id_bytes[..4].copy_from_slice(&state.number_of_deposits.to_le_bytes()); | |
| applied_deposit_id = deposit_id_bytes; | |
| } | |
| if deposit_id == [0u8; 32] { | |
| state.number_of_deposits += 1; | |
| applied_deposit_id[..4].copy_from_slice(&state.number_of_deposits.to_le_bytes()); | |
| } |
We can use simpler form of fixed array comparison and also we can avoid deposit_id_bytes variable by reusing applied_deposit_id that is known to be all zeros in this block
| data.extend_from_slice(&depositor.to_bytes()); | ||
| data.extend_from_slice(&deposit_nonce.to_le_bytes()); | ||
|
|
||
| keccak::hash(&data).0 |
There was a problem hiding this comment.
| keccak::hash(&data).0 | |
| keccak::hash(&data).to_bytes() |
Better not access internal elements
| data.extend_from_slice(&msg_sender.to_bytes()); | ||
| data.extend_from_slice(&depositor.to_bytes()); | ||
| data.extend_from_slice(&deposit_nonce.to_le_bytes()); |
There was a problem hiding this comment.
we might better reuse anchor serializer (should have the same result):
AnchorSerialize::serialize(&(msg_sender, depositor, deposit_nonce), &mut data)?
this though requires converting this function to return Result
Or alternative use borch directly:
let data = (msg_sender, depositor, deposit_nonce).try_to_vec()?
There was a problem hiding this comment.
note, I ended up using AnchorSerialize in this PR
| data.extend_from_slice(&depositor.to_bytes()); | ||
| data.extend_from_slice(&deposit_nonce.to_le_bytes()); | ||
| // Use AnchorSerialize to serialize the tuple of values | ||
| (msg_sender, depositor, deposit_nonce).serialize(&mut data).unwrap(); |
There was a problem hiding this comment.
note that this is not AnchorSerialize wrapper (like mentioned in comment above), but using borch directly. Though, I guess functionally its the same in this case.
This PR adds the logic defined in this PR: #756 to the solana implementation.
there is one gotcha and opinionated change that was needed to make this work: deposit_id is now a
[u8;32]to accommodate this change. practically, this is not really a big deal for relayers or data worker but should be noted.