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

How to handle SIGN_MODE_TEXTUAL spec updates #15291

Closed
Tracked by #11970
amaury1093 opened this issue Mar 7, 2023 · 1 comment · Fixed by #15270
Closed
Tracked by #11970

How to handle SIGN_MODE_TEXTUAL spec updates #15291

amaury1093 opened this issue Mar 7, 2023 · 1 comment · Fixed by #15270

Comments

@amaury1093
Copy link
Contributor

Summary

Define how to handle SIGN_MODE_TEXTUAL spec updates.

Problem Definition

We allow SIGN_MODE_TEXTUAL's spec to be updated, as state-machine-breaking changes. However, we need to define how to coordinate these updates between node and clients.

Take an example: imagine SDK v0.48 uses SIGN_MODE_TEXTUAL v1 (current spec), but v0.49 uses v2 (small value renderer formatting change). If a JS client uses v1 to generate to sign doc and the signature, and sends to a v2 node (or vice versa), then we'll get the usual hard-to-debug message:

signature verification failed; verify nonce, acct num and chain id.

Find a solution that:

  1. Allows coordination between clients and node on which spec version to use
  2. Allows easy debugging of failed signatures

Proposal

During yesterday's Textual sync, multiple solutions were discussed:

  • Proposals 1a and 1b: put the version info in the Tx itself
  • Proposals 2a and 2b: put the version info into the Textual SignDoc

➡️ Proposal 1a: Use SIGN_MODE_TEXTUAL_V{N}

Create a new sign mode with a version suffix each time there's a spec change. The first one can be SIGN_MODE_TEXTUAL, but subsequent ones are _V2 etc.

  • Pros:
    • solves 1 & 2 in the easiest possible way
  • Cons:
    • multiple Textual sign modes, slight code duplication

In this case, we should also probably disallow one node being able to activate two different SIGN_MODE_TEXTUAL versions at the same time.

➡️ Proposal 1b: Add a new field in the Tx struct

Similar to Proposal 1a, but put the version somewhere else in the Tx, e.g.

  message Single {
    // mode is the signing mode of the single signer
    cosmos.tx.signing.v1beta1.SignMode mode = 1;

+   // Additional info for the sign mode
+   string additional_info = 2;
  }
  • Pros:
    • solves 1 & 2
    • we keep one single SIGN_MODE_TEXTUAL enum
    • backwards compatible
  • Cons:
    • modification of the Tx struct

➡️ Proposal 2a: Put the version in the Textual sign doc (not shown to user)

After #15282, Textual's sign doc is the CBOR encoding of:

type SignDoc struct {
  screens []Screen
}

it would be easy to add a version uint64 field there.

  • Pros:
    • solves 2, debuggability is slightly worse than 1a and 1b, but still doable
    • one SIGN_MODE_TEXTUAL enum
  • Cons:
    • problem 1 needs a separate solution, though should be easy to implement (e.g. a simple mapping SDK version <-> Textual spec Version should suffice for JS clients, or in the future, a gRPC endpoint).

➡️ Proposal 2b: Show the spec version as an expert screen

Same as 2b, but put the spec version in one of the Ledger screens (expert mode only).

  • Pros:
    • slight improvement over 2a's debuggability
  • Cons:
    • additional screen
@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label Mar 7, 2023
@amaury1093 amaury1093 added S:proposed and removed needs-triage Issue that needs to be triaged labels Mar 7, 2023
@amaury1093 amaury1093 changed the title SIGN_MODE_TEXTUAL spec updates How to handle SIGN_MODE_TEXTUAL spec updates Mar 7, 2023
@github-project-automation github-project-automation bot moved this to 📝 Todo in Cosmos-SDK Mar 7, 2023
@amaury1093 amaury1093 moved this from 📝 Todo to 💪 In Progress in Cosmos-SDK Mar 7, 2023
@github-project-automation github-project-automation bot moved this from 💪 In Progress to 👏 Done in Cosmos-SDK Mar 20, 2023
@tac0turtle tac0turtle removed this from Cosmos-SDK May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants
@amaury1093 and others