Conversation
yrong
force-pushed
the
ron/multi-assets-transfer
branch
from
f9d6a6c to
ea1f2dd
Compare
yrong
force-pushed
the
ron/multi-assets-transfer
branch
from
b77bc39 to
1af53a2
Compare
| export type AggregatedAsset = { | ||
| tokenErcMetadata: ERC20Metadata | ||
| ahAssetMetadata: Asset | ||
| sourceAssetMetadata: Asset | ||
| amount: bigint | ||
| } |
There was a problem hiding this comment.
Given that we don't want to change the SDK interface too often, can we add a field to distinguish if an asset is an ERC-20 or PNA, so we don't have to change the interface later? Or does this type already support this?
We don't have to add PNA support right now, but the interface should be defined with PNA in mind so we don't need to make unnecessary changes later.
There was a problem hiding this comment.
An Asset with a non-nil location field is treated as PNA; otherwise, it’s ENA.
It’s a bit implicit, though — we might consider adding an explicit flag to make this clearer.
There was a problem hiding this comment.
I think a flag would be cool yeah! :)
| if (isNativeBalance && fee.totalFeeInNative) { | ||
| if (amount + fee.totalFeeInNative > tokenBalance) { | ||
| logs.push({ | ||
| kind: ValidationKind.Error, | ||
| reason: ValidationReason.InsufficientTokenBalance, | ||
| message: "Insufficient token balance to submit transaction.", | ||
| }) | ||
| } | ||
| } else { | ||
| if (amount > tokenBalance) { | ||
| logs.push({ | ||
| kind: ValidationKind.Error, | ||
| reason: ValidationReason.InsufficientTokenBalance, | ||
| message: "Insufficient token balance to submit transaction.", | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we not leave these basic token balances checks? Dry run doesn't always give a clear error to the user.
There was a problem hiding this comment.
I’d prefer to remove it for simplicity. It can be detected by a dry run, and the result should already include the error code, for example:
error: {
Module: {
index: 31
error: 0x1c000900
}
}
I assume this can then be further decoded into a more detailed error description.
@alistair-singh What's your take on this?
There was a problem hiding this comment.
I would like to remove them for simplicity. However the UI is driven by these errors. We use them for many cases such as for NeuroWeb, when the API returns InsufficientTokenBalance we pop up a pre transfer step which allows the user to wrap/unwrap.
We do similar for ETH. So I would only support it if we can get the same kind of granularity.
| const sourceParachainImpl = await paraImplementation(parachain) | ||
| const { tokenErcMetadata, sourceParachain, ahAssetMetadata, sourceAssetMetadata } = | ||
| resolveInputs(registry, tokenAddress, sourceParachainImpl.parachainId) | ||
| resolveInputs(registry, tokens[0].address, sourceParachainImpl.parachainId) |
There was a problem hiding this comment.
Should add a check if tokens.length > 1, throw an error that only 1 PNA is supported at this time.
…rk/snowbridge into ron/multi-assets-transfer
3 participants
Context
Adds support for multiple-asset transfers in the P→E direction, along with some refactoring and cleanup. For now, since PNA is not yet widely adopted in production, this implementation supports only multiple ERC-20 assets. We can extend it in a future PR if necessary.
Resolves: https://linear.app/snowfork/issue/SNO-1637