Skip to content

chore: set refund on destination as the default #2150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
melisaguevara wants to merge 7 commits into
base: master
Choose a base branch
from
Open

chore: set refund on destination as the default #2150

melisaguevara wants to merge 7 commits into from
+155 −24

Conversation

@melisaguevara
Copy link
Contributor

@melisaguevara melisaguevara commented Jan 7, 2026 •
edited
Loading

Aiming to reduce the amount of refunded deposits, this PR sets refund on destination as the default behavior when a destination swap is required.

Note: FE is already explicitly setting refundOnOrigin to true (here) which is the behavior we want for requests coming from our FE.

Tests from API:

Test from FE:

@vercel
Copy link

vercel bot commented Jan 7, 2026 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
app-frontend-v3 Ready Ready Preview, Comment Jan 8, 2026 1:59pm
sepolia-frontend-v3 Ready Ready Preview, Comment Jan 8, 2026 1:59pm

@melisaguevara melisaguevara force-pushed the mg/default-destination-refund branch from c36ae18 to 6aadbdc Compare January 7, 2026 20:26
@vercel
Copy link

vercel bot commented Jan 7, 2026

Deployment failed with the following error:

Hobby accounts are limited to daily cron jobs. This cron expression (* * * * *) would run more than once per day. Upgrade to the Pro plan to unlock all Cron Jobs features on Vercel.

Learn More: https://vercel.link/3Fpeeb1

@melisaguevara melisaguevara marked this pull request as draft January 7, 2026 21:01
@melisaguevara melisaguevara marked this pull request as ready for review January 7, 2026 21:54
dohaki
dohaki reviewed Jan 8, 2026
View reviewed changes
Copy link
Contributor

@dohaki dohaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! The flaky e2e tests are still a bit annoying though... I got misled by the failure reason because this change results in successful fills. We should therefore explicitly set refundOnOrigin=true in the e2e tests.

@melisaguevara
Copy link
Contributor Author

melisaguevara commented Jan 8, 2026 •
edited
Loading

This looks great! The flaky e2e tests are still a bit annoying though... I got misled by the failure reason because this change results in successful fills. We should therefore explicitly set refundOnOrigin=true in the e2e tests.

@dohaki good call! I implemented that e2ab7cb and e5abbac
Tests still fail but now the fill is not being executed.

I considered adding a destination refund test, but I'm not sure there's a reliable way to deterministically cause a swap to fail now that the API throws when slippage is set too low. So I decided to skip that for now.

@melisaguevara melisaguevara requested a review from dohaki January 8, 2026 14:19
dohaki
dohaki approved these changes Jan 8, 2026
View reviewed changes
Copy link
Contributor

@dohaki dohaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dohaki dohaki dohaki approved these changes

@NikolasHaimerl NikolasHaimerl NikolasHaimerl approved these changes

@mrice32 mrice32 Awaiting requested review from mrice32 mrice32 is a code owner

@nicholaspai nicholaspai Awaiting requested review from nicholaspai nicholaspai is a code owner

@pxrl pxrl Awaiting requested review from pxrl pxrl is a code owner

@amateima amateima Awaiting requested review from amateima amateima is a code owner

@ashwinrava ashwinrava Awaiting requested review from ashwinrava ashwinrava is a code owner

@gsteenkamp89 gsteenkamp89 Awaiting requested review from gsteenkamp89 gsteenkamp89 is a code owner

@0xjorgen 0xjorgen Awaiting requested review from 0xjorgen 0xjorgen is a code owner

Assignees

No one assigned

Labels

do not merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@melisaguevara @dohaki @NikolasHaimerl