feat: depositNow #712
feat: depositNow #712
Conversation
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
| @@ -79,7 +74,7 @@ pub fn deposit_v3( | |||
| exclusive_relayer: Pubkey, | |||
| quote_timestamp: u32, | |||
| fill_deadline: u32, | |||
| exclusivity_deadline: u32, | |||
| exclusivity_period: u32, | |||
There was a problem hiding this comment.
we had miss-named this prop! the implementation was correct but the variable name was wrong. see
contracts/contracts/SpokePool.sol
Line 489 in 31693cd
There was a problem hiding this comment.
I think we had it correct initially, but EVM implementation had changed since here
There was a problem hiding this comment.
on master EVM implementation it's still called exclusivityPeriod
so I think as it is now we are consistent?
| // overflow (from devnet testing). add a test to re-create this and fix it such that the error is thrown, | ||
| // not caught via overflow. | ||
| if current_time - quote_timestamp > state.deposit_quote_time_buffer { | ||
| if current_time.checked_sub(quote_timestamp).unwrap_or(u32::MAX) > state.deposit_quote_time_buffer { |
There was a problem hiding this comment.
if checked_sub underflows then it defaults to the unwrap_or value, of u32::MAX, forcing the if to return true, and throwing the error. This fixes the TODO above that I saw on testnet (and validated in unit tests).
There was a problem hiding this comment.
note that EVM implementation has the same underflow issue, so we might fix it there too
There was a problem hiding this comment.
in the evm implementation if it fails at that point does it revert with an underflow error or does it throw the associated revert message that we are expecting?
There was a problem hiding this comment.
as per this comment, it would underflow in EVM
There was a problem hiding this comment.
right but my question was would it throw the error InvalidQuoteTimestamp or underflow. it seems it will throw with underflow (see here).
my original point is that this is actually a bit confusing for a caller. there are situations where multiple invalid input values return the same error. equally, there are situations where some invalid input values, depending on the input, return inconsistent errors.
I agree with your original reply that we should fix this consistently, changing the evm implementation such that all invalid errors return the same error.
| @@ -17,12 +17,7 @@ use crate::{ | |||
| output_token: Pubkey, | |||
| input_amount: u64, | |||
| output_amount: u64, | |||
| destination_chain_id: u64, // TODO: we can remove some of these instructions props | |||
There was a problem hiding this comment.
we dont need the extra fields here in the instructions. also, be removing them, we could re-use this DepositV3 context for the depositNow method.
There was a problem hiding this comment.
We def better call deposit_v3 from deposit_v3_now, same as its done in EVM
There was a problem hiding this comment.
Originally before removing the extra instructions, I was not able to re-call deposit_v3. I've now tried again and with the removed extra instructions (in particular, the ones missing when calling deposit_v3_now) I was able to re-use the original deposit_v3 method. take a look again.
| const fillDeadline = new BN(Math.floor(Date.now() / 1000) + 600); // 600 seconds from now. | ||
| const exclusivityDeadline = new BN(Math.floor(Date.now() / 1000) + 300); // 300 seconds from now. | ||
| const exclusivityPeriod = new BN(300); // 300 seconds. |
There was a problem hiding this comment.
this update applies to match the EVM changes and the correct input prop and how it's meant to be used.
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
| if !ctx.accounts.route.enabled { | ||
| return err!(CommonError::DisabledRoute); | ||
| } |
There was a problem hiding this comment.
This is redundant as this uses DepositV3 context that already checked the route.
There was a problem hiding this comment.
and we better not reimplement the deposit_v3 logic here, but call it internally
test/svm/SvmSpoke.Deposit.ts
Outdated
| @@ -15,7 +15,7 @@ const { provider, connection, program, owner, seedBalance, initializeState, depo | |||
| const { createRoutePda, getVaultAta, assertSE, assert, getCurrentTime, depositQuoteTimeBuffer, fillDeadlineBuffer } = | |||
| common; | |||
|
|
|||
| describe("svm_spoke.deposit", () => { | |||
| describe.only("svm_spoke.deposit", () => { | |||
Signed-off-by: chrismaree <[email protected]>
| @@ -134,45 +134,22 @@ pub fn deposit_v3_now( | |||
| message: Vec<u8>, | |||
| ) -> Result<()> { | |||
| let state = &mut ctx.accounts.state; | |||
There was a problem hiding this comment.
state does not need to be mut here anymore as we only use it for getting current time.
There was a problem hiding this comment.
pub fn get_current_time(_state: &State) expects mutable state for this function. we might want to remove this requirement for this as it does not need to mutate it.
There was a problem hiding this comment.
I don't think get_current_time needs mut state, but we would need to pass the borrowed reference, e.g.:
let current_time = get_current_time(&ctx.accounts.state)?;
Signed-off-by: chrismaree <[email protected]>
Signed-off-by: chrismaree <[email protected]>
Adds Deposit Now, fixes 2 todos and addresses one small bug found.