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

Use Signed Message for Key Devrivation #1981

Merged
merged 11 commits into from
Dec 13, 2024
Merged

Use Signed Message for Key Devrivation #1981

merged 11 commits into from
Dec 13, 2024

Conversation

mj850
Copy link
Contributor

@mj850 mj850 commented Dec 8, 2024

Describe your changes and provide context

Currently, the CT Module relies directly on the private key for deriving keys. For each account, a new keypair is deterministically generated using the KeyGen(privateKey, denom) method, which computes the SHA256 of the privateKey with denom as the salt, then uses that hash to create a private key (random number)

The benefit of doing this is that users can generate a key that is as secure as their addresses' ecdsa private key, without having to manage additional keys.

This is fine for clients such as seid which have direct access to the users private key, but the majority of other clients (wallets etc), do not have direct access to the users private key. This would make the feature very difficult to use outside of seid.

To make things easier for wallets, the change proposed here is that instead of using SHA256Hash(privatekey, denom) as the key generation function, we use Sign(privateKey, denom), then SHA256 hash it one more time with an arbitrary salt to prevent the signature from being used.

Sign(privateKey, denom) is as secure, since it cannot be generated by anyone except with knowledge of the ecdsa private key. However, this enables the key generation to be performed by Wallets, enabling applications to derive the keyPair on the client side by requesting that the user sign the denom. This will increase the future usability of this feature and integratability with dapps.

On the client side, they can generate Sign(privateKey, denom) using EVM wallet clients with

        const message = "tokenfactory/address/denom"

        // Hash the prefixed message
        const messageBytes = new TextEncoder().encode(message);
        const hash = keccak256(messageBytes);

        signatureHex = signMessage({ message: hash})

One note is that this implementation only enables EVM wallets to do this - cosmos wallets have different signing methods and will still not be able to interact with this module naturally.

more work to be done to look into how to add usability to this module.

This PR is dependent on the changes in sei-protocol/sei-cryptography#7

Testing performed to validate your change

  • Unit tests/integration tests updated - module still works as expected
  • Did a POC to make sure the hex signature produced by viem client matches the one produced by the new GetSignedDenom method

mj850 added 3 commits December 7, 2024 23:47
mj850 added 2 commits December 8, 2024 03:14

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link

codecov bot commented Dec 8, 2024

Codecov Report

Attention: Patch coverage is 58.82353% with 28 lines in your changes missing coverage. Please review.

Project coverage is 60.99%. Comparing base (5ff5ab3) to head (f27876c).
Report is 10 commits behind head on feature/ct_module.

Files with missing lines Patch % Lines
x/confidentialtransfers/utils/keygen.go 79.16% 8 Missing and 2 partials ⚠️
x/confidentialtransfers/types/transfer.go 28.57% 5 Missing ⚠️
...nfidentialtransfers/types/apply_pending_balance.go 0.00% 4 Missing ⚠️
.../confidentialtransfers/types/initialize_account.go 0.00% 4 Missing ⚠️
x/confidentialtransfers/types/withdraw.go 0.00% 4 Missing ⚠️
x/confidentialtransfers/types/close_account.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                  Coverage Diff                  @@
##           feature/ct_module    #1981      +/-   ##
=====================================================
+ Coverage              60.89%   60.99%   +0.09%     
=====================================================
  Files                    283      284       +1     
  Lines                  26982    27029      +47     
=====================================================
+ Hits                   16431    16485      +54     
+ Misses                  9289     9280       -9     
- Partials                1262     1264       +2     
Files with missing lines Coverage Δ
x/confidentialtransfers/types/close_account.go 0.00% <0.00%> (ø)
...nfidentialtransfers/types/apply_pending_balance.go 0.00% <0.00%> (ø)
.../confidentialtransfers/types/initialize_account.go 0.00% <0.00%> (ø)
x/confidentialtransfers/types/withdraw.go 0.00% <0.00%> (ø)
x/confidentialtransfers/types/transfer.go 54.39% <28.57%> (ø)
x/confidentialtransfers/utils/keygen.go 79.16% <79.16%> (ø)

... and 4 files with indirect coverage changes

mj850 added 3 commits December 8, 2024 12:07

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Contributor

@dssei dssei left a comment

Choose a reason for hiding this comment

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

lgtm

@mj850 mj850 merged commit 0999e20 into feature/ct_module Dec 13, 2024
28 of 29 checks passed
@mj850 mj850 deleted the mj/ctSign branch December 13, 2024 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants