-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Minor nits
expect(parseInt(order.info.input.amount.toString())).to.be.greaterThan(90000000); | ||
expect(parseInt(order.info.input.amount.toString())).to.be.lessThan(110000000); | ||
|
||
const { tokenInBefore, tokenInAfter, tokenOutBefore, tokenOutAfter } = await baseTest.executeRelaySwap( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wanted to avoid on-chain integ tests here as they tend to be flaky and slow - can we move these to SDK integ instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I had that thought too but I think we need tests here to test that the calldata for UR is being built correctly - there's not a great way to test that in the SDK level because the classic quote (which the calldata) is generated from is only available at the quote level within URA. Also, we don't mainnet fork for the SDK integ tests so we'd have to do that.
I kept these e2e tests simple and short - only two cases, and both doing stable to stable with reasonable bounds. I'll remove the checks that the input amounts are within an explicit range so the only remaining checks are relative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, logic generally lgtm
This PR wires up relay quotes into the existing handlers and adds e2e tests. I've kept the traditional style of integration tests (fetch quote, execute calldata, check amounts) because we are generating universal router calldata within URA for relay quotes and its important that it's generated correctly. These integration tests require that and should also not be very flaky as they are only between USDC and USDT and check min/max values.
Other changes: