Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

EIP 191 #9701

Merged
merged 6 commits into from
Nov 14, 2018
Merged

EIP 191 #9701

merged 6 commits into from
Nov 14, 2018

Conversation

seunlanlege
Copy link
Member

implemented EIP-191 signing (personal_sign191)

@parity-cla-bot
Copy link

It looks like this contributor signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Oct 5, 2018
@sorpaas sorpaas added this to the 2.2 milestone Oct 5, 2018
@5chdn 5chdn requested review from tomusdrw and andresilva and removed request for tomusdrw October 26, 2018 11:30
@5chdn
Copy link
Contributor

5chdn commented Oct 26, 2018

please rebase on master @seunlanlege

please review this well

@seunlanlege
Copy link
Member Author

actually this depends on #9631

@5chdn 5chdn modified the milestones: 2.2, 2.3 Oct 29, 2018
@tomusdrw
Copy link
Collaborator

Unreviewable in current state, we need to get #9631 merged first.

@ordian
Copy link
Collaborator

ordian commented Oct 31, 2018

please rebase on master

@5chdn
Copy link
Contributor

5chdn commented Nov 1, 2018

#9631 is merged, please rebase

@seunlanlege
Copy link
Member Author

done

@5chdn
Copy link
Contributor

5chdn commented Nov 1, 2018

does not compile

@seunlanlege
Copy link
Member Author

rebase went wrong, fixed it now

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

looks good, but adding docs and tests never hurts :)

rpc/src/v1/impls/personal.rs Outdated Show resolved Hide resolved
use serde::de;
use v1::types::{H160, Bytes};

pub enum EIP191Version {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please document all public types/fields/functions

Copy link
Collaborator

Choose a reason for hiding this comment

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

missing docs for variants

rpc/src/v1/helpers/eip191.rs Outdated Show resolved Hide resolved
@ordian ordian added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 1, 2018
@Tbaut
Copy link
Contributor

Tbaut commented Nov 1, 2018

Can you please add the function to the jsonrpc repo just like with eip712?

@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Nov 7, 2018
@ordian ordian requested a review from tomusdrw November 13, 2018 14:21
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good! What standard this method corresponds to?
It seems that here: https://github.com/ethereum/go-ethereum/pull/17789/files we have a different approach (it uses stringy content-type instead of versions as hex).

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(
f,
"sign 0x{} with {}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change the previous implementation to distinguish between eip191 and this?

use serde::de;
use v1::types::{H160, Bytes};

pub enum EIP191Version {
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing docs for variants

@seunlanlege
Copy link
Member Author

@seunlanlege seunlanlege merged commit a8617e2 into master Nov 14, 2018
@seunlanlege seunlanlege deleted the EIP-191 branch November 14, 2018 08:02
seunlanlege added a commit that referenced this pull request Nov 14, 2018
* added sign_191 rpc method

* fixed hash_structured_data return type

* added ConfirmationPayload::SignMessage for non-prefixed signatures, added tests for sign191

* renamed WithValidator -> PresignedTransaction

* rename applicationData to data in test

* adds docs for EIP191Version, renamed SignRequest to EIP191SignRequest
This was referenced Nov 14, 2018
@Tbaut Tbaut added A8-looksgood 🦄 Pull request is reviewed well. B1-patch-beta 🕷🕷 and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 14, 2018
@andresilva
Copy link
Contributor

Is personal_sign191 the method name other implementations are going to use? I couldn't find anything on it.

@seunlanlege
Copy link
Member Author

Is personal_sign191 the method name other implementations are going to use? I couldn't find anything on it.

In clef, it's currently implemented as signData,

https://github.com/ethereum/go-ethereum/pull/17789/files#diff-f8316f6ee7e75303007a55a1cde29245R50

Tbaut added a commit that referenced this pull request Nov 14, 2018
* Bump beta to version 2.2.1

* fix: Intermittent failing CI due to addr in use (#9885)

Allow OS to set port at runtime

* Use Weak reference in PubSubClient (#9886)

* Fix json tracer overflow (#9873)

* Fix json tracer overflow

* Replace trace_executed with a direct trace push

* Remove unused variable

* Add test for 5a51

* Remove duplicate json!

* Fix docker script (#9854)


* Dockerfile: change source path of the newly added check_sync.sh (#9869)

* Allow to seal work on latest block (#9876)

* Allow to seal work on latest block.

* Test from @todr to check sealing conditions.

* gitlab-ci: make android release build succeed (#9743)

* use docker cargo config file for android builds

* make android build succeed

* ethcore: use Machine::verify_transaction on parent block (#9900)

* ethcore: use Machine::verify_transaction on parent block

also fixes off-by-one activation of transaction permission contract

* ethcore: clarify call to verify_transaction

* foundation: #6692865, ropsten: #4417537, kovan: #9363457

* Remove rust-toolchain file (#9906)

* EIP-712 implementation (#9631)

* EIP-712 impl

* added more tests

* removed size parsing unwrap

* corrected TYPE_REGEX to disallow zero sized fixed length arrays, replaced LinkedHashSet with IndexSet, added API spec to docs, fixed Type::Byte encoding branch

* use Option<u64> instead of u64 for Type::Array::Length

* replace `.iter()` with  `.values()`

Co-Authored-By: seunlanlege <[email protected]>

* tabify eip712.rs

* use proper comments for docs

* Cargo.lock: revert unrelated changes

* tabify encode.rs

* EIP 191 (#9701)

* added sign_191 rpc method

* fixed hash_structured_data return type

* added ConfirmationPayload::SignMessage for non-prefixed signatures, added tests for sign191

* renamed WithValidator -> PresignedTransaction

* rename applicationData to data in test

* adds docs for EIP191Version, renamed SignRequest to EIP191SignRequest

* light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure (#9824)

* fix start_gas, handle OOG exceptions & NotEnoughGas

* Change START_GAS: 50_000 -> 60_000
* When the `OutOfGas exception` is received then try to double the gas until it succeeds or block gas limit is reached
* When `NotEnoughBasGas error` is received then use the required gas provided in the response

* fix(light-fetch): ensure block_gas_limit is tried

Try the `block_gas_limit` before regard the execution as an error

* Update rpc/src/v1/helpers/light_fetch.rs

Co-Authored-By: niklasad1 <[email protected]>

* simplify cargo audit

* Use block header for building finality (#9914)

* ci: nuke the gitlab caches (#9855)
@5chdn 5chdn added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Nov 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants