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

[DO NOT MERGE] 378 create a pr to add pendulum assets #2

Closed
wants to merge 3 commits into from

Conversation

gianfra-t
Copy link

@gianfra-t gianfra-t commented Aug 28, 2024

As before the objective of this PR is not to merge but to check it. Later we can merge to upstream Talisman/chaindata repository.

Related: Tasks/#378.

Some of the required tokens on the issue where already included on the Pendulum configuration. This adds:

  • USDC
  • BRZ
  • PINK
  • HDX
  • ASTR (ED taken from our definitions in orml_tokens pallet definition)
  • vDOT
  • BNC
  • USDC.axl

Also, adds ED for existing DOT and USDT.

@gianfra-t gianfra-t force-pushed the 378-create-a-pr-to-add-pendulum-assets branch from 1bc4186 to 1616980 Compare August 28, 2024 13:13
@gianfra-t gianfra-t requested a review from a team August 28, 2024 21:16
@gianfra-t gianfra-t changed the title 378 create a pr to add pendulum assets [DO NOT MERGE] 378 create a pr to add pendulum assets Aug 28, 2024
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Looks good to me 👌

@gianfra-t
Copy link
Author

Great, I will close this PR and open the same to the Talisman repo.

@gianfra-t gianfra-t closed this Aug 29, 2024
{
"symbol": "BNC",
"decimals": 12,
"ed": "10000000000",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I misunderstand how this definition works but the ED of all our non-native assets is 1000. This also applied to vDOT.

Copy link
Member

Choose a reason for hiding this comment

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

True, but I think this differs when you register non-native assets from other chains. They usually tell us the existential deposit of their asset that we should use for registration. And that's why some tokens have a non-1000 ED also in our asset registry metadata. And this change would just reflect that.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, but this is then just a definition in the registry, not the actual ED. The actual ED is only determined by orml-tokens and there it is 1000 for each asset. Should we change this then in order to have consistent definitions? It probably doesn't matter too much but could be confusing otherwise.

Copy link
Member

@ebma ebma Aug 30, 2024

Choose a reason for hiding this comment

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

I think technically speaking you are right that we can allow 1000 on our chain. But from a user perspective, it might be confusing if they send just 1000 units of e.g. BNC to us but can then not send it back because the existential deposit on Binance is set to 10000000000. To avoid this confusion I guess it's best to just re-use the existential deposit that our HRMP partner chains define on their chains. This should be the least confusing to the user and they don't want/need to know about the ED definitions on each parachain.

Copy link
Author

Choose a reason for hiding this comment

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

It may be better to consult what exactly are they using this ED for, if it is only for receiving or also sending, or something else. We can easily open a new PR to double check all our registered assets once we know this.

Copy link
Member

Choose a reason for hiding this comment

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

Even if we set our ED to 10000000000, as long as we don't control that the minimum sending amount is also 10000000000, then anyone who owns 10000000001 tokens on Pendulum could just send 1 token via XCM to an empty target account and that target account would not reach the required ED.

True, but I guess the wallet application would use the information about the existential deposit to prevent the user from doing exactly that.

And then again, even we define in our XCM configuration that the minimum transfer amount is at least the ED, then this might be overly restrictive if sending an amount to an account that already has more than the ED on the target chain.

I think this is the bigger problem. The wallet application could also add extra handling for this though. The wallet can check the balance of the destination account on the other chain and then decide whether the amount that the user is trying to send makes sense or not. I don't know whether Talisman or the other wallet apps are doing this. As @gianfra-t said, we would need to ask the devs of Talisman in order to confirm.

Now you could argue that if the wallet app integrates extra handling to check the existential deposit before transferring (as I suggested in my previous point), then we could also continue using our custom existential deposit that is potentially different from the one on the source chain as it shouldn't matter. As long as we don't know if/how wallets handle the existential deposit, it's difficult to make an informed decision.

Copy link
Member

Choose a reason for hiding this comment

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

I would be very impressed if wallets do this test. It is probably easy to check ourselves whether this is the case or not.

Copy link
Member

Choose a reason for hiding this comment

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

@pendulum-chain/product do we have a communication channel with the Talisman team on Telegram?

Copy link

Choose a reason for hiding this comment

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

yes we have @ebma

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding me to it, I'll ask them later

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.

4 participants