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.

Sign up for GitHub

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

Jump to bottom

Move verify function from wallet.Backend to wallet.Sig #295

Closed
wants to merge 12 commits into from
Closed

Move verify function from wallet.Backend to wallet.Sig #295

wants to merge 12 commits into from

Conversation

manoranjith
Copy link
Contributor

Closes #294.

@manoranjith
💥 [wallet] Change Sig to binary.(Un)marshaler
67a22a2 
- Previously Sig was defined as a variable length byte array and was
  encoded/decoded using the Encode/DecodeSig methods.

- But, its length depends upon the wallet backend. So, redefine it as an
  interface that implements binary.(Un)marshaler interfaces.

- Now, the wallet backend should implement the logic for converting a
  Sig to/from its binary representation. The perunio serializer can then
  directly Encode/Decode this binary representation as a byte array.

Signed-off-by: Manoranjith <[email protected]>
@manoranjith
♻️ [sim/wallet] Update sim wallet to implement Sig interface
f475ad1 
- Use a struct for storing the r and s components of the ECDSA
  signature.

- This eliminates the need for serializing / deserializing the
  signature each time when it is generated / verified respectively.

- Also, replace the test TestSignatureSerialize with
  GenericMarshalerTest

Signed-off-by: Manoranjith <[email protected]>
@manoranjith
♻️ [eth/wallet] Update eth wallet to implement Sig interface
e93d17a 
- Unexport the SigLen field, as it is not needed outside of the wallet
  package.

- Remove the length check in signature tests. Because, VerifySignature
  function checks if the signature was generated by the corresponding
  wallet, before verifying it it.

- In transactor tests, use the sigLen variable defined within the
  package.

Signed-off-by: Manoranjith <[email protected]>
@manoranjith
♻️ [eth/channel] Update eth/channel to use the Sig interface
817acbd 
Signed-off-by: Manoranjith <[email protected]>
@manoranjith
♻️ [client] Update client pkg to use the Sig interface
0dfaf41 
- (En|De)code signatures directly using perunio.(En|De)code.

- SignData method on dummy account returns a Sig instead of byte array.

Signed-off-by: Manoranjith <[email protected]>
@manoranjith
✅ [wallet/test] Update tests to work with Sig interface
cae91e2 
- After the wallet.Sig interface was introduced, each wallet backend
  defines its own concrete type for implementing the Sig interface.

- These implementations also include an UnmarshalBinary function, which
  is used for unmarshalling signatures from their binary representation.

- Because of this, when binary representations of signatures have
  incorrect length (when they are tampered), the unmarshalling of these
  signatures themselves fail. Which removes the need for checking the
  response of VerifySignature function for signatures of incorrect
  length.

- Hence, the tests are updated accordingly.

Signed-off-by: Manoranjith <[email protected]>
@manoranjith
♻️ [persistence/test] Use Sig.Clone for cloning a signature
bf56d1c 
Signed-off-by: Manoranjith <[email protected]>
@manoranjith
Copy link
Contributor Author

manoranjith commented Dec 14, 2021 •
edited
Loading

This PR currently includes changes from #293 as well. So only those commits after be3a590 are within the scope of this PR.

@manoranjith
✅ [persistence/test] Use randomizer to generate signatures
be3a590 
- Previously, signatures used in tests were directly generated as byte
  arrays.

- After the changing type of wallet.Sig from byte array to interface,
  this does not work.

- Hence, use the wallet randomizer to generate signatures needed for
  tests.

Signed-off-by: Manoranjith <[email protected]>
@manoranjith
💥 [wallet] Move VerifySignature from Backend to Sig
5eb91df 
- Previously, VerifySignature function is defined on the wallet backend.

- This looked fine, because until recently, wallet.Sig was defined
  as a byte array.

- But, now it has been redefined as an interface with each wallet
  implementation implementing the wallet.Sig interface.

- Hence, moved the VerifySignature method from wallet.Backend to
  wallet.Sig interface.

Signed-off-by: Manoranjith <[email protected]>
@manoranjith
💥 [sim/wallet] Move VerifySignature from Backend to Sig
db4d98d 
Signed-off-by: Manoranjith <[email protected]>
@manoranjith
💥 [eth/wallet] Move VerifySignature from Backend to Sig
1b08a69 
Signed-off-by: Manoranjith <[email protected]>
@manoranjith
:refactor: [wallet/test] Replace Backend.VerifySignature w/ Sig.Verify
310978a 
Signed-off-by: Manoranjith <[email protected]>
@manoranjith manoranjith force-pushed the 294-move-verify-from-backend-to-sig branch from 87bb5e8 to 310978a Compare December 14, 2021 12:41
@matthiasgeihs
Copy link
Contributor

This is a change to the external API with no clear justification. It is debatable which is the better approach, have the Verify method on the Wallet, or on the Sig.

I have looked at https://pkg.go.dev/crypto/ecdsa and https://github.com/google/tink and both do not have the Verify function on the signature. Instead, they rather have a signature of type []byte and have the Verify function on some central object, either the package or a dedicated Verifier object.

The fact that other libraries tend to treat signatures typically just as []byte makes me wonder whether actually introducing a type Sig is a good idea at all. It seems to introduce a lot of additional types (e.g., now each wallet needs to implement a type Sig). Moreover, you mentioned that the reason for introducing type Sig was separation of concerns for perunio, but now I am wondering: If it is about serializing a signature into bytes, shouldn't that part be trivial if the type of sig is []byte already?

matthiasgeihs
matthiasgeihs reviewed Dec 14, 2021
View reviewed changes
Copy link
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

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

Please see #295 (comment)

@matthiasgeihs matthiasgeihs mentioned this pull request Dec 14, 2021
Closed
@matthiasgeihs
Copy link
Contributor

Closed. See #233 for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiasgeihs matthiasgeihs matthiasgeihs left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move VerifySignature from wallet.Backend to wallet.Sig
2 participants
@manoranjith @matthiasgeihs