Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update auth cli commands #6717
Update auth cli commands #6717
Changes from 60 commits
9b4a4a1
eaeef54
b2c8997
5abd7f1
0a80caa
c2b4cc7
d9bbef8
d1eb8c1
2d5335b
cc2b7e0
688e9cd
d4cbf42
425cbfe
27a57a5
64f7aaf
987af37
167df15
8f82734
baebff4
ed36414
3122b59
57c8798
39bc7f4
1afa4d3
d101c34
012e320
e955ca6
3dec8f7
7a42cfe
ac46abe
ff733fc
30bca25
b390579
695b77d
4be2e51
b4f8c77
6fe5838
467d692
3a0efa5
6d3297e
15613cf
4f2b71a
819aa66
07f22b2
e004e1b
1fe8088
2e2099d
88bdb62
9f04601
013c481
1252fbf
72d3982
f95810a
28bbc0f
5aa68e7
62ff121
5b3c906
6c1a0fa
29dab69
750c5ac
074ee7b
0c94f78
7771328
423cbcb
d9b15df
0136c11
2073e3f
af89c16
c5d7181
c6a7353
ecf0679
8a4c583
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super similar to SignatureV2. Could we remove SignatureV2 and use this one everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine we can. But it will probably be a decent size refactor. Are you fine with that in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have
message ModeInfo
, and if I understand this, this is ModeInfo+Signature. I'm still not 100% aware of protobuf extending types and such, but is there a way to use ModeInfo?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not without adding a field on ModeInfo that will be unused in transactions. The challenge is that you still need to distinguish between single and multisigs.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I would be happy to remove the
SignatureData
interface (which is just an abstraction to representoneof SingleSignatureData | MultiSignatureData
), and just use the concrete type SignatureDescriptor everywhere in the codeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree here. I'd go with what @amaurymartiny stated or at the very least, change the nomenclature because it's confusing and the types clobber each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if for the time-being we rename this function to
SignatureDataToProto
or something like that?And then maybe in a future PR we just have a single proto generated
SignatureData
that is used everywhere as @amaurymartiny is suggesting?This is an example of different encoding/application types btw. I've always questioned that philosophy because of the amount of duplication... Anyway, I think this is a concrete example of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SignatureDataToProto is bit confusing as well. I suggest to keep it as it is now and unblock this PR, and address this in a followup PR as @amaurymartiny suggesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that okay @alexanderbez ? It would be good to get a decision either way because this is one of the last critical pieces to enabling proto tx as the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we leave it as-is, it's super confusing IMHO. I'd prefer
SignatureDataToProto
TBH. You're converting a domain-type to a wire-type (i.e. Proto).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so how about we go with
SignatureDataTo/FromProto
here? And as a follow-up @amaurymartiny you'll take a look at possible improvements?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that sgtm 👍