forked from TalismanSociety/chaindata
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 just1000
units of e.g. BNC to us but can then not send it back because the existential deposit on Binance is set to10000000000
. 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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but I guess the wallet application would use the information about the existential deposit to prevent the user from doing exactly that.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we have @ebma
There was a problem hiding this comment.
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