Skip to content
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.

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

feat: add xcm dry run api #257

Merged
merged 9 commits into from
Jan 23, 2025
Merged

feat: add xcm dry run api #257

merged 9 commits into from
Jan 23, 2025

Conversation

noahjoeris
Copy link
Collaborator

This PR

  • adds automatic dry run when available for xcm transfers

Design Decisions

  • If no dry run is available we treat it as if it has succeeded
  • The dry run returns precise xcm fees. But we run the dry run after the form was filled out and immediately before the tx-sign popup appears. So I decided to not use those fees for now. Alternatively, we can explore running the dry run as part of fetching the fees in txSummary. I would prefer to do that in a seperate PR tho.

Copy link

vercel bot commented Jan 22, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
turtle-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2025 2:40pm
turtle-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2025 2:40pm

@NunoAlexandre
Copy link
Collaborator

@noahjoeris

If no dry run is available we treat it as if it has succeeded

this is not good. Using a Result type here would be best. If it ran, we check for the dry run result, if it failed we can for example capture the error with sentry and continue with the rest of the execution.

The dry run returns precise xcm fees. But we run the dry run after the form was filled out and immediately before the tx-sign popup appears. So I decided to not use those fees for now. Alternatively, we can explore running the dry run as part of fetching the fees in txSummary. I would prefer to do that in a seperate PR tho.

Have you seen how different the xcm fees are? because I'd guess we are talking about a few cents difference at worse? dry running is probably a slow-enough operation to make this idea a no-go. Additionally, the user's wallet will also provide a fee estimate and that should be the last amount the user should consider. So I wouldn't spend time on this and run the risk of worsening the ux.

@noahjoeris
Copy link
Collaborator Author

Could you look at the code? I implemented it how you described no?

@NunoAlexandre
Copy link
Collaborator

Could you look at the code? I implemented it how you described no?

yes but you used booleans and exceptions instead of a well typed Result approach. That's where my suggestion lays.

@noahjoeris
Copy link
Collaborator Author

@NunoAlexandre Can you check again

@noahjoeris
Copy link
Collaborator Author

Thanks for your input guys. I updated it accordingly

Copy link
Collaborator

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

Nice :) thanks for considering my feedback

app/src/hooks/useParaspellApi.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@Victor-Salomon Victor-Salomon left a comment

Choose a reason for hiding this comment

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

Great work @noahjoeris 💥

@noahjoeris noahjoeris merged commit 7f62a6a into main Jan 23, 2025
5 checks passed
@noahjoeris noahjoeris deleted the feat/dry-run branch January 23, 2025 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants