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

Implement recently added signing standards from Parity and Geth #1241

Closed
1 of 4 tasks
tuxxy opened this issue Feb 12, 2019 · 23 comments
Closed
1 of 4 tasks

Implement recently added signing standards from Parity and Geth #1241

tuxxy opened this issue Feb 12, 2019 · 23 comments

Comments

@tuxxy
Copy link

tuxxy commented Feb 12, 2019

See:

An API should be implemented in web3py that allows for flexible utility between node backends with regards to the signing API proposed in the above issues. This API should also not require that the developer handle private key material. This API should rely on the account management of the respective backend node software.

  • Geth EIP-191 API
  • Geth EIP-712 API
  • Parity EIP-191 API
  • Parity EIP-712 API
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 1499.0 DAI (1499.0 USD @ $1.0/DAI) attached to it as part of the nucypher fund.

@gitcoinbot
Copy link

gitcoinbot commented Feb 12, 2019

Issue Status: 1. Open 2. Cancelled


Work has been started.

These users each claimed they can complete the work by 1 week, 6 days from now.
Please review their action plans below:

1) bhargavasomu has been approved to start work.

I have been taking a look at the library for a while, and as far as the issue is concerned I will take care of the local signing (eth-account) and the actual signing happening on the node.

Learn more on the Gitcoin Issue Details page.

@mswilkison
Copy link

Hi @iamonuwa! Can you join our Discord channel to chat about this before we approve? https://discord.gg/7rmXa3S Thanks!

@pipermerriam
Copy link
Member

@mswilkison I'd like someone from my team to weigh in as well as this is going to be a feature that's very important to be right as it falls intot he "security" category. Also, implementation will almost definitely occur under eth-account.

@kclowes would you mind digging around for the other issues related to this as I know we have at least one more open. They might be under eth-account.

@pipermerriam
Copy link
Member

Just read the description and noticed this issue is focused on an implementation that delegates to the underlying connected node, which makes it less critical since web3 will simply be acting as an intermediary. I do think I'd still like a brief write-up of the planned approach before giving someone a 👍 to proceed.

@Bhargavasomu
Copy link
Contributor

@pipermerriam this is the proposal I have in mind for taking this forward.

  • For the local sign (eth-account), version 0x45 (or personal_sign) has already been implemented as far as EIP 191 is concerned. This whole signing part happens at the signature_wrapper function which is used by defunct_hash_message. This defunct_hash_message function doesn't take the signature_version as a parameter as of now. We could pass this parameter to this function, which in turn would call the signature_wrapper function with the appropriate version.
  • In the signature_wrapper function, we would have to add support for these versions which are in hand with the EIPs.
  • Finally in web3.py we would have to change wherever defunct_hash_message is used, so that the signature_version is also included.

Please correct me if I am wrong anywhere.

@pipermerriam
Copy link
Member

@mswilkison I'd like to allocate this bounty to @Bhargavasomu if possible. @iamonuwa thanks for your interest.

@pipermerriam
Copy link
Member

Also @mswilkison , going forward @kclowes from my team will be handling our side of approving bounty stuff for this issue.

@mswilkison
Copy link

Hey @Bhargavasomu! Are you working on this? The bounty is about to expire on gitcoin so I need to know if we should extend it or not - thanks!

@Bhargavasomu
Copy link
Contributor

@mswilkison I have already completed implementing EIP 191. You can take a look here at ethereum/eth-account#56. It might take another 10 days to get the Implementation of EIP 712 to get merged.

I would be delighted if you could extend the expire time to another 10 days.

@mswilkison
Copy link

@Bhargavasomu done. Thanks!

@mswilkison
Copy link

Hey @Bhargavasomu looks like everything here is merged and done?

@Bhargavasomu
Copy link
Contributor

@mswilkison the eth_signTypedData needs to be still implemented in the web3.py repository. It shouldn't take long. Will get back to you after this is completed. Thankyou.

@Bhargavasomu
Copy link
Contributor

@kclowes could you please confirm that this issue and bounty is done? Thankyou.

@kclowes
Copy link
Collaborator

kclowes commented May 2, 2019

Confirmed. Thanks @Bhargavasomu!

@kclowes kclowes closed this as completed May 2, 2019
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Cancelled


The funding of 1499.0 DAI (1499.0 USD @ $1.0/DAI) attached to this issue has been cancelled by the bounty submitter

@kclowes
Copy link
Collaborator

kclowes commented May 2, 2019

Uh oh, I hope I didn't cancel the bounty by closing the PR
issue. Let me know what I need to do @mswilkison

@mswilkison
Copy link

Sorry, something is wrong with gitcoin (or I did something wrong). Getting them to help. Stand by @Bhargavasomu

@gitcoinbot
Copy link

⚡️ A tip worth 1499.00000 DAI (1499.0 USD @ $1.0/DAI) has been granted to @Bhargavasomu for this issue from @mswilkison. ⚡️

Nice work @Bhargavasomu! Your tip has automatically been deposited in the ETH address we have on file.

@tuxxy
Copy link
Author

tuxxy commented May 3, 2019

It just occurred to us that #1319 only partially fulfills the needs that this issue established.

Currently awaiting Geth to implement the necessary API endpoints, which is fine. But #1319 only implements Parity EIP-712 signatures, but we also require EIP-191 signatures. As we were a bit busy with our own development, we didn't notice the disparity in this until now.

We also need to expose the API endpoint for Parity's personal_sign191 endpoint, which is available now.

With that said, can we reopen this issue?

@kclowes
Copy link
Collaborator

kclowes commented May 3, 2019

Yep, for sure. I didn't realize there was a specific 191 endpoint.

@kclowes kclowes reopened this May 3, 2019
@Bhargavasomu
Copy link
Contributor

@tuxxy sorry about that, I will open a new PR to add the personal_sign191 endpoint.

@kclowes
Copy link
Collaborator

kclowes commented Jan 29, 2024

It appears that 191 is only implemented via Clef in Geth, so no JSON-RPC endpoint is needed within web3.py. We do have EIP-191 signing implemented in eth-account via sign_message, exposed in web3 via w3.eth.account.sign_message. Closing since I don't think there is work to do within web3, but if anyone feels differently, please open a new issue.

@kclowes kclowes closed this as completed Jan 29, 2024
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

No branches or pull requests

6 participants