Skip to content

feat: Check Destination sufficiency with an optional destApi#565

Closed
andrew-ifrita wants to merge 16 commits into
mainfrom
feat/optional-dest-api
Closed

feat: Check Destination sufficiency with an optional destApi#565
andrew-ifrita wants to merge 16 commits into
mainfrom
feat/optional-dest-api

Conversation

@andrew-ifrita

@andrew-ifrita andrew-ifrita commented May 20, 2025

Copy link
Copy Markdown
Contributor

If provided use destApi to make additional calls to ensure that receiving accounts will be sufficient and that no assets will be lost when accounts are reaped.

This is somewhat of a band-aid, as accidental loss of assets is still possible at the protocol level. This at least improves the user experience when it comes to transferring assets until we can investigate fixing the root of the issue at the protocol level.

General Idea

Throw and error or raise a warning if the receiving account will not be sufficient following the transaction.

Motivation

Make it harder for assets to be accidentally be burned by sending them in insufficient accounts which will be reaped.

Goal

  • Don't break any existing API
  • Verify receiving accounts are (or will be) sufficient
  • Warn users

Downsides

  • Verification only happens at the time of the tx building
    • future account changes could still result in assets potentially being lost if the destinations are no longer sufficient
  • Additional API calls

Questions

  • Do we want to throw and error and kill the tx building, or simply warn the user?
    • If we throw an error, should we add an optional bypass to ignore these checks

@andrew-ifrita andrew-ifrita marked this pull request as draft May 20, 2025 11:48
@andrew-ifrita andrew-ifrita force-pushed the feat/optional-dest-api branch 3 times, most recently from d377b34 to 4263f49 Compare May 21, 2025 15:13
If provided, make additional calls to ensure that receiving accounts
will be sufficient and that no assets will be lost when accounts are
reaped.
@andrew-ifrita andrew-ifrita force-pushed the feat/optional-dest-api branch from 4263f49 to da304af Compare May 22, 2025 09:21
@andrew-ifrita andrew-ifrita force-pushed the feat/optional-dest-api branch from 6bff899 to 941174c Compare May 22, 2025 10:29
Also updated .eslintignore as it was taking forever
@andrew-ifrita andrew-ifrita changed the title feat: Add optional destApi argument to createTransferTransaction feat: Check Destination sufficiency with an optional destApi May 22, 2025
@andrew-ifrita andrew-ifrita marked this pull request as ready for review May 22, 2025 12:22
@andrew-ifrita

Copy link
Copy Markdown
Contributor Author

I can add a few tests, but it should be good enough for review at this point.

@andrew-ifrita andrew-ifrita requested a review from TarikGul May 22, 2025 12:23
@andrew-ifrita

andrew-ifrita commented May 28, 2025

Copy link
Copy Markdown
Contributor Author

Reviewing this a few days later: I would say I'm missing MultiAddress support.

Similarly, (probably an edge case), but the pattern here won't work if sending to multiple destination chains. I need to investigate that a little. We could add a mapping of destination API to asset, but I'm not a fan of the UX. Although it would be mostly consistent with what currently exists and UX could be improved afterwards edit: actually while possible with XCM, this breaks the current design since we explicitly provide a single destChainId

@andrew-ifrita andrew-ifrita force-pushed the feat/optional-dest-api branch from 51332a9 to 829500b Compare May 30, 2025 14:33
@andrew-ifrita

andrew-ifrita commented May 30, 2025

Copy link
Copy Markdown
Contributor Author

Ok, discovered one more issue which is accounting for XCM execution fees being deducted along the way which can lead to a transfer being below the existential deposit and then being lost to account reaping. This is a blocker. Test case is already in place, Relay chain to Asset Hub throws error if destination is insufficient after execution fees.

I have yet to add foreign asset support, but that is less pressing, as it would only cause false positives on the insufficiency check which won't lead to loss of funds, unlike the false negatives if fees are not accounted for.

edit: Moving to draft in the meantime

@andrew-ifrita andrew-ifrita marked this pull request as draft June 3, 2025 07:25
@andrew-ifrita

Copy link
Copy Markdown
Contributor Author

Closing as this has gone stale.

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.

1 participant