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

go/common/crypto/signature/signers/ledger: Descriptive error on user reject #3050

Merged
merged 1 commit into from
Jun 25, 2020

Conversation

tjanez
Copy link
Member

@tjanez tjanez commented Jun 25, 2020

Make Ledger signer return a more descriptive error message when a user rejects a transaction on the Ledger device.

For example, previously, the returned error was:

ts=2020-06-25T10:26:59.930888297Z level=error module=cmd/common/consensus caller=consensus.go:107 msg="failed to sign transaction" err="[APDU_CODE_COMMAND_NOT_ALLOWED] Command not allowed (no current EF)"

Now, it is:

ts=2020-06-25T10:27:24.693936203Z level=error module=cmd/common/consensus caller=consensus.go:107 msg="failed to sign transaction" err="transaction was rejected on Ledger device"

@tjanez tjanez added the c:cli Category: command line interface label Jun 25, 2020
@tjanez tjanez force-pushed the tjanez/ledger-err-reject-tx branch 2 times, most recently from a92a699 to 639a1b5 Compare June 25, 2020 10:28
@tjanez tjanez changed the title go/oasis-node/cmd/common/consensus: More descriptive error on Ledger reject go/common/crypto/signature/signers/ledger: Descriptive error on user reject Jun 25, 2020
@tjanez tjanez removed the c:cli Category: command line interface label Jun 25, 2020
return s.device.SignEd25519(s.path, preparedContext, message)
signature, err := s.device.SignEd25519(s.path, preparedContext, message)
if err != nil {
if err.Error() == ledgerRejectTxErrorMsg {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use errors.Is instead. Also consider using a switch statement instead of these nested ifs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed nested ifs and used a switch statement.

I couldn't use errors.Is() because at the moment, ledger-go doesn't use proper Go error semantics and doesn't expose errors as variables.

@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #3050 into master will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3050      +/-   ##
==========================================
- Coverage   68.27%   68.23%   -0.04%     
==========================================
  Files         372      372              
  Lines       36437    36444       +7     
==========================================
- Hits        24877    24868       -9     
- Misses       8346     8365      +19     
+ Partials     3214     3211       -3     
Impacted Files Coverage Δ
...n/crypto/signature/signers/ledger/ledger_signer.go 14.00% <0.00%> (-2.28%) ⬇️
go/oasis-node/cmd/common/metrics/disk.go 65.51% <0.00%> (-20.69%) ⬇️
go/consensus/tendermint/api/errors.go 86.66% <0.00%> (-13.34%) ⬇️
...nsensus/tendermint/apps/keymanager/transactions.go 44.11% <0.00%> (-8.83%) ⬇️
go/oasis-node/cmd/common/metrics/resource.go 84.00% <0.00%> (-8.00%) ⬇️
...nsensus/tendermint/apps/staking/signing_rewards.go 21.62% <0.00%> (-5.41%) ⬇️
...n/crypto/signature/signers/memory/memory_signer.go 71.42% <0.00%> (-4.77%) ⬇️
.../consensus/tendermint/apps/epochtime_mock/state.go 78.72% <0.00%> (-4.26%) ⬇️
go/consensus/tendermint/apps/staking/state/gas.go 77.58% <0.00%> (-3.45%) ⬇️
go/consensus/tendermint/roothash/roothash.go 63.87% <0.00%> (-2.61%) ⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5373604...bb7fbe7. Read the comment docs.

@tjanez tjanez force-pushed the tjanez/ledger-err-reject-tx branch from 639a1b5 to a0b89d7 Compare June 25, 2020 16:17
Make Ledger signer return a more descriptive error message when a user
rejects a transaction on the Ledger device.
@tjanez tjanez force-pushed the tjanez/ledger-err-reject-tx branch from a0b89d7 to bb7fbe7 Compare June 25, 2020 16:20
@tjanez tjanez merged commit 925b7f2 into master Jun 25, 2020
@tjanez tjanez deleted the tjanez/ledger-err-reject-tx branch June 25, 2020 19:50
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.

None yet

2 participants