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
[DDW-663] Use asset policyId+name as unique identifier instead of the fingerprint #2562
[DDW-663] Use asset policyId+name as unique identifier instead of the fingerprint #2562
Conversation
…nique-identifier-instead-of-the-fingerprint
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.
Hi @daniloprates, is the reason for this change that since there is not enough screen space to show the full fingerprint, the AssetId may as well be used?
No @rvl - this is in order to ensure uniqueness in terms of Daedalus internal asset data handling. So far we treated the fingerprint as the unique identifier which is a false assumption. We are doing this to avoid any future conflicts. |
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.
Great work 🎉 I just added a few comments 👍
source/renderer/app/components/wallet/send-form/WalletSendAssetsConfirmationDialog.js
Show resolved
Hide resolved
…nique-identifier-instead-of-the-fingerprint
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.
LGTM. Tested on 18005 👍
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.
Code looks great 👍
@daniloprates I will review and merge this tomorrow morning! |
@gabriela-ponce please make sure to test this one 🙏 |
@daniloprates This looks good, but I find something that I'm not sure if it's expected and I couldn't reproduce it in develop:
|
Thanks @gabriela-ponce! Fixed. |
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.
👍
Hi, |
@dsc81 please submit a feature request here https://github.com/input-output-hk/daedalus/issues |
This PR changes the assets identifiers from fingerprint to
policyId+name
.Todos
policyId+name
as asset identifiersTesting Checklist
Test Cases
Review Checklist
Basics
feature
/bug
/chore
,release-x.x.x
)yarn test
)yarn dev
)yarn package
/ CI builds)yarn flow:test
)yarn lint
)yarn prettier:check
)yarn manage:translations
produces no changes)yarn storybook
)yarn.lock
file is updatedCode Quality
Testing
After Review
done
column on the YouTrack board