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

Add eth_getPlumeSignature RPC handler #198

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

rrrliu
Copy link

@rrrliu rrrliu commented Feb 21, 2023

Context: Implementation of a novel deterministic ECDSA signature scheme in MetaMask https://eprint.iacr.org/2022/1255

Add handler for eth_getPlumeSignature to unblock MetaMask/metamask-extension#17482, as we'd like to use this new RPC method to generate the signature + other necessary SNARK inputs.

@rrrliu rrrliu requested a review from a team as a code owner February 21, 2023 10:17
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

The test coverage for the wallet middleware is lacking, however, we should try to make sure we add tests for additions we make going forward. Can you see if you can do that? Unfortunately we don't have a great example you can work off of, but you can kind of see how this works for existing RPC methods, e.g. personal_sign:

it('should sign with a valid address', async () => {
. You'll want to add a new describe block to this file and test out the behavior of eth_getPlumeSignature (as least as far as you can go). Can you do that? Let me know if you need help.

@Gudahtt
Copy link
Member

Gudahtt commented Feb 21, 2023

We should ensure that the extension team wishes to accept this change before we include it here. Just wanted to give you a heads up before you do more work on these tests, that it might be better to get buy-in from that team first.

@Divide-By-0
Copy link

Divide-By-0 commented Feb 21, 2023

We should ensure that the extension team wishes to accept this change before we include it here. Just wanted to give you a heads up before you do more work on these tests, that it might be better to get buy-in from that team first.

Great point! We have already discussed with Alex Bea, Erikson, Kumavis, and Olaf to confirm their buy-in, along with our Metamask grant proposal. Feel free to tag any of them here to confirm!

@shanejonas
Copy link
Contributor

Could you add the method as a PR to our api-specs repo as well: https://github.com/MetaMask/api-specs

@rrrliu
Copy link
Author

rrrliu commented Feb 23, 2023

Thank you both for the great feedback!

@legobeat legobeat requested a review from mcmire August 22, 2023 14:26
@mcmire mcmire removed their request for review May 21, 2024 15:10
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.

6 participants