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

Allow additional/override of non-spec fields, including "address" #70

Merged
merged 3 commits into from
Aug 12, 2024

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Aug 8, 2024

As discussed in #69:

  • The address field is not part of the Version 3 spec - was removed here justification:

    Address unnecessary and compromises privacy.

  • The address field is a very Ethereum specific way to store a piece of metadata that is intended to identify the private key publicly
    • An 0x prefixed 20 hex-byte compressed identifier of the SECP256K1 derivied public key, for the private key
  • When this library is used to stored key materials for other curves/cryptogrpahy (as now supported since Expose API to directly call HashStruct from EIP-712 and M+N dims #68) its misleading

So...

This PR allows WalletFile.Metadata() to be called get a map[string]any that you can used to:

  1. store arbitrary additional fields, like bjjIdentifer or btcIdentifier that are different public key representations of the private key
  2. remove the address field from the serialization by setting it to nil

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Copy link
Contributor

@jimthematrix jimthematrix left a comment

Choose a reason for hiding this comment

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

LGTM, approved with a suggestion that can be pushed off to a followup PR.


type walletFileBase struct {
walletFileCoreFields
walletFileMetadata
privateKey []byte
keypair *secp256k1.KeyPair
Copy link
Contributor

Choose a reason for hiding this comment

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

would be great to have this removed as well, to make the walletFile object agnostic to the public key algorithms. this was done in #69. but no need to hold the merge for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - agreed... I wonder if I can make it calculate-on-demand as a way to keep the interface identical 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great suggestion @jimthematrix - I've managed to do that I think completely removing the field, and even actually removing the Address struct field.

I realized we don't expose the field directly anyway.

Mind taking another look?

…nctions

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@jimthematrix
Copy link
Contributor

thanks @peterbroadhurst , think this is the best we can do given that KeyPair() *secp256k1.KeyPair was part of the package API already. LGTM

pkg/secp256k1/keypair.go Outdated Show resolved Hide resolved
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst peterbroadhurst merged commit 7e256d7 into main Aug 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants