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

ledger wallet API: support sending full tx msgpack to device #436

Merged
merged 3 commits into from Oct 29, 2019

Conversation

@zeldovich
Copy link
Contributor

zeldovich commented Oct 27, 2019

This allows us to add support for additional transaction features (like
note fields or new asset transaction types) without having to keep
modifying kmd. Fallback to older format of transaction encoding if the
Ledger device is running an old version of the Algorand app.

zeldovich added 2 commits Oct 27, 2019
This allows us to add support for additional transaction features (like
note fields or new asset transaction types) without having to keep
modifying kmd.  Fallback to older format of transaction encoding if the
Ledger device is running an old version of the Algorand app.
Copy link
Contributor

tsachiherman left a comment

I would generally be against adding new features, but this one doesn't really interact with any of the others, so I guess there is no reason to block it from going to the upcoming release, right ?


// Error satisfies builtin interface `error`
func (err LedgerUSBError) Error() string {
return fmt.Sprintf("unexpected status %x", err)

This comment has been minimized.

Copy link
@tsachiherman

tsachiherman Oct 29, 2019

Contributor

nit: change it to Exchange: unexpected status 0x%x so that we'll know where this issue came from as well as making it easier to decode.

This comment has been minimized.

Copy link
@zeldovich

zeldovich Oct 29, 2019

Author Contributor

Good point. Pushed; can you re-approve?

@zeldovich

This comment has been minimized.

Copy link
Contributor Author

zeldovich commented Oct 29, 2019

I would generally be against adding new features, but this one doesn't really interact with any of the others, so I guess there is no reason to block it from going to the upcoming release, right ?

Indeed, this code seems highly orthogonal to anything else going on in the codebase, so even in the worst case, it should at most break support for the Ledger wallet.

Copy link
Contributor

tsachiherman left a comment

Looks great. thanks.

@tsachiherman tsachiherman merged commit f7fd042 into algorand:master Oct 29, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
license/cla Contributor License Agreement is signed.
Details
winder added a commit that referenced this pull request Oct 31, 2019
* ledger wallet API: support sending full tx msgpack to device

This allows us to add support for additional transaction features (like
note fields or new asset transaction types) without having to keep
modifying kmd.  Fallback to older format of transaction encoding if the
Ledger device is running an old version of the Algorand app.

* make golint happy

* improve string format of LedgerUSBError
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.