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

PLT-9086: Enable Hardware wallet Signing (canonical CBOR format) #794

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jhbertra
Copy link
Contributor

@jhbertra jhbertra commented Jan 4, 2024

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Test report is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
    • Operables are updated with changes to executable command line options.
    • Deploy charts updated with changes to operables.
  • PR
    • Self-reviewed the diff
    • Useful pull request description
      • Review required
      • Substantial changes to code, test, or documentation
      • Change made to Marlowe validator (@bwbush and @palas must be included as reviewers)
      • Review not required
      • Minor changes to non-critical code, documentation, nix derivations, configuration files, or scripts
      • Formatting, spelling, grammar, or reorganization
    • Reviewer requested

@jhbertra jhbertra force-pushed the plt-9086-wallet-compatible-cbor branch from cc4417c to 6ab39ee Compare January 5, 2024 16:16
@jhbertra jhbertra marked this pull request as ready for review January 5, 2024 17:59
@bwbush
Copy link
Collaborator

bwbush commented Jan 5, 2024

To test this properly with hardware wallets, we might need a Runner or TS SDK that is compatible with this branch. @paluh, @nhenin, @hrajchert, what are the prospects for that?

@hrajchert
Copy link
Collaborator

We could prioritize this as a new goal for the TS-SDK, but either Nicolas or myself would need access to a hardware wallet

@jhbertra
Copy link
Contributor Author

jhbertra commented Jan 8, 2024

To start with, we also need to make sure the software wallets are still compatible - note that this has a breaking change that will require a code change in the TS SDK ("type": "ShelleyTxWitness BabbageEra" -> "type": "TxWitness BabbageEra")

@hrajchert
Copy link
Collaborator

When will this be published? it would be nice if we can send the right envelope depending on the backend version

@jhbertra
Copy link
Contributor Author

jhbertra commented Jan 8, 2024

It will be published in a non-breaking way to a new host. As for supporting multiple backend versions, that's up to you and @nhenin if you want to go down that route. Might not work though, as newer request formats likely won't be back-portable.

@bwbush bwbush self-requested a review January 8, 2024 13:16
@paluh
Copy link
Collaborator

paluh commented Jan 8, 2024

@bwbush I think that I should test this anyway in the Runner and check the impact on the CIP-30 wallets using our playwright based test suite. We can discuss this during sprint planning.

Copy link
Collaborator

@bwbush bwbush left a comment

Choose a reason for hiding this comment

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

Blocked until we confirm with compatibility with Lace, Eternl, and Nami.

@bwbush
Copy link
Collaborator

bwbush commented Jan 8, 2024

Attached is the debug log from Eternl using a Ledger wallet to sign an apply-inputs transaction.
eternl-debug-1704737725810.json. A similar error occurs for a Trezor wallet.

@bwbush
Copy link
Collaborator

bwbush commented Jan 8, 2024

I manually checked our unsigned transaction. The keys are not in canonical order. The hardware wallets probably choke on this [1] [2]. Runtime needs to canonicalize the CBOR and conform to RFC 8949 and RFC 7049.

I see at least two problems in our CBOR, but there may be more:

  1. Some map keys are not sorted.
  2. Some lists have a terminator instead of fixed length.

unsigned.cbor.txt
unsigned.txt
unsigned.json

…086_wallet_compatible_cbor.md

Co-authored-by: Brian W Bush <[email protected]>
@nhenin
Copy link
Contributor

nhenin commented Feb 5, 2024

@jhbertra, sorry I missed the notifications on that... what is the status now ? Did you fix the 2 hypothesis ?

- Some map keys are not sorted.
- Some lists have a terminator instead of fixed length.

@nhenin nhenin changed the title Plt 9086 wallet compatible cbor PLT-9086: Enable Hardware wallet Signing (canonical CBOR format) Feb 5, 2024
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.

5 participants