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

Make keystorev3 wallets curve-agnostic #69

Closed

Conversation

jimthematrix
Copy link
Contributor

@jimthematrix jimthematrix commented Aug 8, 2024

Make the implementation in the pkg/keystorev3 package agnostic of public key curves, and only manages the private key bytes with encryption and decryption in persistence.

Ideally the pkg/fswallet is also made curve agnostic, but that's the main API used by downstream client code and the Ethereum's 20-byte address is already entrenched. For a different system like Babyjubjub, in particular the implementation in https://github.com/iden3/go-iden3-crypto, the public key compression format is 32 bytes. So the right approach for now is build a parallel implementation of the fswallet somewhere else

@jimthematrix jimthematrix marked this pull request as draft August 8, 2024 17:58
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
@jimthematrix jimthematrix marked this pull request as ready for review August 8, 2024 18:10
Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

In #68 I already put code to make the keystore agnostic - you can just store/retrieve arbitrary bytes.

The one weirdness is that there is an "address" JSON entry in the keystorev3 - it's not really needed or used for anything as far as I can tell, just a convenience to store something in the file that uniquely identifies the key material.... and it happens to use SECP256K1 (because it was the Eth community that designed keystorev3).

So I suspect it is the "wrongess" of the address string for BabyJubJub that you're really pushing at with this PR, is that right?

@peterbroadhurst
Copy link
Contributor

... if that is the case, I think the best thing would be to make the interface more generic for someone to explicitly set the address field on the WalletFile if they don't want to use the 20-byte compression syntax of Ethereum.

That could be callback function?

Then something like https://github.com/iden3/go-iden3-crypto library dependence could be pushed out of this base utility, into the calling code.

@peterbroadhurst
Copy link
Contributor

I think maybe it's easiest if I provide some code to state what I mean @jimthematrix - so I'll pop that into a code proposal.

@jimthematrix
Copy link
Contributor Author

Then something like https://github.com/iden3/go-iden3-crypto library dependence could be pushed out of this base utility, into the calling code.

that's already the case as you can see the BJJ specific logic is only in the test code. not inside keystorev3 at all. as is the case with the secp256k1 specific logic.

@peterbroadhurst
Copy link
Contributor

I do also note that the Ethereum standard actually removed address after Version 1 - so we're not actually spec compliant having it, but all implementations seem to have kept it:

https://github.com/ethereum/wiki/wiki/Web3-Secret-Storage-Definition#alterations-from-version-1

Address unnecessary and compromises privacy.

@jimthematrix
Copy link
Contributor Author

Closing this per the comments above

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.

2 participants