Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Use pubkey as identifier in SignatureSetType #1193

Merged
merged 5 commits into from
Apr 26, 2018
Merged

Use pubkey as identifier in SignatureSetType #1193

merged 5 commits into from
Apr 26, 2018

Conversation

l4l
Copy link
Contributor

@l4l l4l commented Apr 9, 2018

Description of the Change

ed25519 can have different signature for the same public key, so it necessary to guarantee uniqueness of each public key in signature set

Signed-off-by: Eugene Minibaev <mail@kitsu.me>
@l4l l4l added needs-review pr awaits review from maintainers bug bug/defect that was reproduced by maintainers labels Apr 9, 2018
@l4l l4l requested review from lebdron and x3medima17 April 9, 2018 07:12
@neewy
Copy link
Contributor

neewy commented Apr 12, 2018

This fix should contain test with text data based on modified version of ed25519

@luckychess luckychess self-requested a review April 13, 2018 10:53
Copy link
Contributor

@luckychess luckychess left a comment

Choose a reason for hiding this comment

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

Looks good overall but I agree with @neewy that we need some tests for this issue.

@l4l
Copy link
Contributor Author

l4l commented Apr 13, 2018

It is not really trivial to write the test for that case, as long as:

  1. requires modification of iroha-ed25519
  2. or there's a need somehow skip timeout check (that is not trivial at all)

@lebdron
Copy link
Contributor

lebdron commented Apr 16, 2018

@l4l What kind of timeout check do you mean? It should be possible to create a transaction with TestTransactionBuilder, call addSignature with modified ed25519 signature, and pass this transaction as an input for some unit or integration test.

@l4l
Copy link
Contributor Author

l4l commented Apr 16, 2018

Good idea

Signed-off-by: Kitsu <mail@kitsu.me>
@neewy
Copy link
Contributor

neewy commented Apr 25, 2018

Any progress? I've noticed you have added test in commit above, is it sufficient to review this fix?

@l4l
Copy link
Contributor Author

l4l commented Apr 25, 2018

Yes, for now only that test can be done without significant projects changes and I think better review&merge it asap, then think about other components (though, they will work as well, as they have code duplication actually)

Signed-off-by: Kitsu <mail@kitsu.me>
Signed-off-by: Kitsu <mail@kitsu.me>
@l4l l4l merged commit 1c639d7 into develop Apr 26, 2018
@l4l l4l deleted the fix/signature_set branch April 26, 2018 16:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug bug/defect that was reproduced by maintainers needs-review pr awaits review from maintainers
Development

Successfully merging this pull request may close these issues.

None yet

5 participants