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.

Sign up for GitHub

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

Jump to bottom

Add Transfer2/Approve2/MintTo2/Burn2 instructions for improved hardware wallet support #343

Merged
merged 4 commits into from
Aug 28, 2020
Merged

Add Transfer2/Approve2/MintTo2/Burn2 instructions for improved hardware wallet support #343

merged 4 commits into from
Aug 28, 2020

Conversation

mvines
Copy link
Member

@mvines mvines commented Aug 27, 2020 •
edited
Loading

TODO:

Depends-on: #342
Depends-on: #315
Fixes: #289

@mvines mvines requested a review from t-nelson August 27, 2020 19:59
@mvines
Copy link
Member Author

mvines commented Aug 27, 2020

@CriesofCarrots - this is ready for review I think. ./coverage.sh says I hit all my new code paths.

@t-nelson - this baby is for you. Can you review to ensure it'll work for you in Ledgerland. I don't see why not but more eyes here is better anyway

@mvines mvines mentioned this pull request Aug 27, 2020
Closed
CriesofCarrots
CriesofCarrots previously approved these changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Code lgtm! Do we want to add docs in the Instruction enum explaining how each Instruction2 differs from the original?

@mvines
Copy link
Member Author

mvines commented Aug 27, 2020

Code lgtm! Do we want to add docs in the Instruction enum explaining how each Instruction2 differs from the original?

Done

@mergify mergify bot dismissed CriesofCarrots’s stale review August 27, 2020 23:21

Pull request has been modified.

t-nelson
t-nelson previously approved these changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

This looks like it'll give me what I need to offer a better token amount UX on the Ledger App.

Are we still envisioning the token symbol strings being kept in an on-device LUT?

@mvines
Copy link
Member Author

mvines commented Aug 27, 2020

Are we still envisioning the token symbol strings being kept in an on-device LUT?

Yeah. We'll just need a hard coded list

@mergify mergify bot dismissed t-nelson’s stale review August 27, 2020 23:37

Pull request has been modified.

@mvines mvines added the automerge Merge this pull request automatically once CI and reviews are compelte label Aug 28, 2020
@mergify mergify bot merged commit 142e8e2 into solana-labs:master Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-nelson t-nelson t-nelson left review comments

@CriesofCarrots CriesofCarrots CriesofCarrots left review comments

Assignees
No one assigned
Labels
automerge Merge this pull request automatically once CI and reviews are compelte
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

token: Consider adding an expected decimals input to all token instructions that deal with an amount
3 participants
@mvines @t-nelson @CriesofCarrots