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
fix: Signature only flag bug on tx sign command 7632 #8106
fix: Signature only flag bug on tx sign command 7632 #8106
Changes from 35 commits
351c28f
4308f1c
942d954
86e1ca7
50568a4
09984c6
b97a939
7d96902
a0d26fb
ac324b3
1b16901
9d5ce97
e774b31
e731b5a
4784d93
2048a8a
b1e3cbd
edc19dc
8818f64
7e39258
3ed4d36
cd6b700
fe3acef
c2c74c6
82d28b8
87d027a
b8256a8
a00e8ec
c0c1c48
6405295
9b9b9dc
d61f340
8cbcff1
25e4915
17f210b
46b50b2
8e55e13
a263570
68d9b37
febbca9
ae2ce73
1407caa
d71fe5a
df7be40
94c42a0
48659f8
5547aaa
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.
There may be something slightly more complicated going on here that we need to be careful about. If you read above, we are only calling this
SetSignatures()
method with an empty sig so that the underlyingtxBuilder
can set theSignModes
which is necessary for generating the bytesToSign below.In the case of
--append
flag, I think that we actually want to callSetSignatures()
with the pre-existing list of existing signatures + the empty new signature.e.g. i think the full overwrite logic here needs to be something more like:
Would be good to get confirmation from @amaurymartiny but i think this is correct logic.
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.
I've added a test to verify the matching signatures. It works. The reason the signatures are reset here is to set the right signing mode.
The
wrapper
and client signing functions are not very clear. As I wrote in the summary above, it should probably be rewritten.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.
I think the edge case that might be missing here is if you have two different signatures with different sign modes. Does the test work in that case as well?
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.
I've added one more tests for signing with a different factory which has a different mode:
In fact your patch doesn't work, because the
wrapper
(TxBuilder) is wrongly implemented - it doesn't know which key it should use to sign nor the mode to use. That logic is handled independently, throughtxFactory
and maybe totally different then the mode set in theTxBuilder
signature placeholder. In my opinion this is error prone, but that's something for a new task.