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

fix: normalize transferred tokens by decimals #159

Merged
merged 17 commits into from
Jan 9, 2025
Merged

Conversation

karim-en
Copy link
Collaborator

@karim-en karim-en commented Dec 19, 2024

Problem:

On Solana, the max normal decimal of the SPL token is 9 and the u64 is used for the transferred token amount, but on the Near side, the tokens usually have 24 decimals which prevents us from bridging this kind of tokens.

Solition:

  • Deploy tokens with decimals 18 on EVM and 9 Solana.
  • Normolise and denormolise the transferred amount on the Near side.

@karim-en karim-en requested review from olga24912, kiseln and frolvanya and removed request for olga24912 December 19, 2024 22:31
@kiseln
Copy link
Contributor

kiseln commented Dec 20, 2024

Can we avoid usign near and do normalization/denormalization purely on destination chains?

For example, when we deploy on a destination chain we store original decimals and if it's higher than the threshold value we do transformations on each init/finalize

@karim-en
Copy link
Collaborator Author

Can we avoid usign near and do normalization/denormalization purely on destination chains?

For example, when we deploy on a destination chain we store original decimals and if it's higher than the threshold value we do transformations on each init/finalize

We can, but more changes will be needed + we will have a problem with the dust remainder which affect the total supply.
In this implementation the dust will be received by relayer with the fee.

near/omni-bridge/src/lib.rs Outdated Show resolved Hide resolved
@kiseln
Copy link
Contributor

kiseln commented Dec 20, 2024

Can we avoid usign near and do normalization/denormalization purely on destination chains?
For example, when we deploy on a destination chain we store original decimals and if it's higher than the threshold value we do transformations on each init/finalize

We can, but more changes will be needed + we will have a problem with the dust remainder which affect the total supply. In this implementation the dust will be received by relayer with the fee.

I understand the dust issue but otherwise I see my suggested approach to be way simpler unless I'm missing something

@karim-en
Copy link
Collaborator Author

Can we avoid usign near and do normalization/denormalization purely on destination chains?
For example, when we deploy on a destination chain we store original decimals and if it's higher than the threshold value we do transformations on each init/finalize

We can, but more changes will be needed + we will have a problem with the dust remainder which affect the total supply. In this implementation the dust will be received by relayer with the fee.

I understand the dust issue but otherwise I see my suggested approach to be way simpler unless I'm missing something

Actually, I've tried to implement it like that, and found that is implementation become too complex, so I moved all this logic just to Near side.
Anyway, feel free to try to implement POC, maybe you will be able to write it better than I did.

@kisialiou
Copy link
Contributor

kisialiou commented Dec 20, 2024

@karim-en Just in case, while the changes are approved, please, pay attention to the fact that EVM tests failed and the Rust build failed as well.

near/omni-bridge/src/lib.rs Outdated Show resolved Hide resolved
near/omni-bridge/src/lib.rs Outdated Show resolved Hide resolved
near/omni-bridge/src/lib.rs Outdated Show resolved Hide resolved
let destination_nonce =
self.get_next_destination_nonce(init_transfer.recipient.get_chain());
let transfer_message = TransferMessage {
origin_nonce: init_transfer.origin_nonce,
token: init_transfer.token,
amount: init_transfer.amount,
amount: Self::de_normalize_amount(init_transfer.amount.0, decimals).into(),
recipient: init_transfer.recipient,
Copy link
Contributor

Choose a reason for hiding this comment

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

denormalize would be one word

@@ -44,7 +44,8 @@ library BridgeTypes {
string token,
string name,
string symbol,
uint8 decimals
uint8 decimals,
uint8 originDecimals
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need originDecimals? Since we have token we can look it up on Near

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a little bit complicated to retrieve the origin decimals due to async specific of the cross calls on Near, so by adding this we can reduce the number of callbacks on Near side

@@ -86,6 +88,8 @@ impl<'info> DeployToken<'info> {
let payload = DeployTokenResponse {
token: metadata.token,
solana_mint: self.mint.key(),
decimals: metadata.decimals,
origin_decimals,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@karim-en karim-en merged commit d54dbc4 into main Jan 9, 2025
7 of 9 checks passed
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.

5 participants