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

Removing usage of sequence in SignatureV2 #8308

Closed
4 tasks
sahith-narahari opened this issue Jan 12, 2021 · 7 comments
Closed
4 tasks

Removing usage of sequence in SignatureV2 #8308

sahith-narahari opened this issue Jan 12, 2021 · 7 comments
Labels

Comments

@sahith-narahari
Copy link
Contributor

Summary

The current implementation of signatureV2 requires having the correct sequence value set, which can be relaxed to promote a more friendly ux

Problem Definition

Using sequence to set the signature needs us to query the node for sequence value even after we have the signature and sign bytes.
This is also causing us issues in rosetta where the construction combine endpoint has sign bytes and signature but still needs to query the node to fetch the sequence value. This prevents from using current rosetta workflow from working in a complete offline mode.

Is it possible to eliminate use of sequence here, would like to discuss the consequences this change has.

cc\ @alessio @aaronc

Proposal


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alessio
Copy link
Contributor

alessio commented Jan 12, 2021

@amaury1093
Copy link
Contributor

amaury1093 commented Jan 12, 2021

SignatureV2 is an application logic type, which is easier to manipulate than the raw proto SignerInfo and bytes signatures.

Are you proposing:

  1. To remove sequence in SignerInfo and SignatureV2?
  2. Only remove sequence in SignatureV2?

If you're proposing 1, then it's a huge client-breaking change. I would advocate against. Initially sequence was not in SignatureV2 (or equivalently, in proto-language, in SignerInfo), then it was decided to be added in #6966

If you're proposing 2, then it seems to me that the conversion SignerInfo->SignatureV2 would lead to loss of information (the sequence field), and I have difficulties seeing how it would work. That being said, I do agree signing types are a bit confusing, and raised #6896 a while ago, so maybe there could be some work done there.

@amaury1093
Copy link
Contributor

amaury1093 commented Jan 12, 2021

Using sequence to set the signature needs us to query the node for sequence value even after we have the signature and sign bytes.

Why? The sequence is already included in the sign_bytes (in SIGN_MODE_DIRECT), you can always extract it from those bytes if needed.

@sahith-narahari
Copy link
Contributor Author

SignatureV2 is an application logic type, which is easier to manipulate than the raw proto SignerInfo and bytes signatures.

Are you proposing:

1. To remove `sequence` in SignerInfo and SignatureV2?

2. Only remove `sequence` in SignatureV2?

If you're proposing 1, then it's a huge client-breaking change. I would advocate against. Initially sequence was not in SignatureV2 (or equivalently, in proto-language, in SignerInfo), then it was decided to be added in #6966

If you're proposing 2, then it seems to me that the conversion SignerInfo->SignatureV2 would lead to loss of information (the sequence field), and I have difficulties seeing how it would work. That being said, I do agree signing types are a bit confusing, and raised #6896 a while ago, so maybe there could be some work done there.

I agree 1 is something we can't afford at this point of time and my proposal was about 2. Looks like we have come a full circle.

With respect to fetching the sequence from sign_bytes, I don't think it's straight forward(as sequence is part of authinfo) but will take a look to see if we can do this.

@robert-zaremba
Copy link
Collaborator

Could you point to a related code for the following issue:

Using sequence to set the signature needs us to query the node for sequence value even after we have the signature and sign bytes.

Removing Sequence from SignatureV2 is rather a bad idea and would lead to a new construction of SignatureV3

@robert-zaremba
Copy link
Collaborator

Maybe SignatureV3 could be an interface ;)

@tac0turtle
Copy link
Member

we introduced a unordered tx type that would allow this to work from my understanding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants