EIP-712 implementation #9631

seunlanlege · 2018-09-24T10:35:49Z

closes #9424

Missing

encode nested array types

parity-cla-bot · 2018-09-24T10:35:52Z

It looks like @seunlanlege signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

util/EIP-712/src/lib.rs

tomusdrw

Looks pretty good, couple of minor style grumbles, and couple security issues (disambiguation), that I haven't seen mentioned explicitly in the spec though.

tomusdrw · 2018-09-24T10:47:42Z

Cargo.toml

@@ -135,7 +135,7 @@ members = [
 	"util/triehash-ethereum",
 	"util/keccak-hasher",
 	"util/patricia-trie-ethereum",
-	"util/fastmap",
+	"util/fastmap"


We prefer to keep trailing commas - so that every line is the same and you can easily add/remove or reorder them.

util/EIP-712/Cargo.toml

util/EIP-712/src/eip712.rs

tomusdrw · 2018-09-24T11:20:45Z

util/EIP-712/src/lib.rs

+		match &*field.type_ {
+			// Array type e.g uint256[], string[]
+			ty if ty.rfind(']') == Some(ty.len() - 1) => {
+				let array_type = ty.split('[').collect::<Vec<_>>()[0];


You don't need to collect(), just use first() and handle possible error (cause what if there is ] in the type but not [?) or maybe better just take a substr up to position of [

util/EIP-712/src/lib.rs

util/EIP-712/src/eip712.rs

util/EIP-712/src/lib.rs

5chdn · 2018-09-29T20:15:07Z

Can you set git user name and mail?

https://help.github.com/articles/setting-your-commit-email-address-in-git/

seunlanlege · 2018-10-01T07:24:34Z

@5chdn my git configs are set

5chdn · 2018-10-01T08:15:42Z

Can you associate the email with your Github account? It does not show your identity for commits

seunlanlege · 2018-10-01T09:22:40Z

ok done.

Cargo.lock

tomusdrw

looks good!

util/EIP-712/Cargo.toml

util/EIP-712/src/lib.rs

tomusdrw · 2018-10-02T09:29:06Z

util/EIP-712/src/lib.rs

+mod parser;
+use parser::*;
+pub use error::*;
+pub use eip712::*;


pub use should be a separate section. Also can we avoid wildcard imports? Is there really so many things there that they can't be listed?

Maybe it's better to just make all the modules (parser, error, eip712) public and just use more explicit path instead?

util/EIP-712/src/lib.rs

util/EIP-712/src/eip712.rs

util/EIP-712/src/parser.rs

ordian

There are some submodules changes still.

util/EIP-712/src/eip712.rs

util/EIP-712/src/encode.rs

util/EIP-712/src/parser.rs

util/EIP-712/Cargo.toml

util/EIP-712/src/parser.rs

util/EIP-712/src/encode.rs

util/EIP-712/src/eip712.rs

util/EIP-712/src/encode.rs

rpc/src/v1/traits/personal.rs

util/EIP-712/src/eip712.rs

util/EIP-712/src/encode.rs

tomusdrw · 2018-10-10T13:43:21Z

util/EIP-712/src/lib.rs

@@ -0,0 +1,303 @@
+extern crate serde;


Could you add unused_extern_crates as well?

util/EIP-712/Cargo.toml

util/EIP-712/src/parser.rs

5chdn · 2018-10-26T11:25:40Z

@seunlanlege please rebase this on master

@tomusdrw @ordian could you give this a final review?

Cargo.lock

util/EIP-712/src/error.rs

util/EIP-712/src/eip712.rs

util/EIP-712/src/encode.rs

tomusdrw

Lgtm, let's get it merged :)

Cargo.lock

util/EIP-712/src/eip712.rs

util/EIP-712/src/parser.rs

…aced LinkedHashSet with IndexSet, added API spec to docs, fixed Type::Byte encoding branch

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

* 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

* 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)

seunlanlege changed the title ~~(WIP) EIP-712 implementation Missing [] encode array types [] json schema validation logic [] some primitive ethabiv1 types are missing, can't seem to find a comprehensive list of them~~ (WIP) EIP-712 implementation Sep 24, 2018

seunlanlege commented Sep 24, 2018

View reviewed changes

util/EIP-712/src/lib.rs Outdated Show resolved Hide resolved

tomusdrw reviewed Sep 24, 2018

View reviewed changes

5chdn added this to the 2.2 milestone Sep 25, 2018

5chdn added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Sep 25, 2018

seunlanlege changed the title ~~(WIP) EIP-712 implementation~~ EIP-712 implementation Oct 1, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 1, 2018

seunlanlege force-pushed the EIP-712 branch from e95b996 to 476302d Compare October 2, 2018 08:44

ordian reviewed Oct 2, 2018

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

tomusdrw reviewed Oct 2, 2018

View reviewed changes

ordian reviewed Oct 2, 2018

View reviewed changes

util/EIP-712/src/parser.rs Show resolved Hide resolved

seunlanlege changed the title ~~EIP-712 implementation~~ EIP-191/EIP-712 implementation Oct 3, 2018

seunlanlege force-pushed the EIP-712 branch 2 times, most recently from 05526ca to 08344e0 Compare October 4, 2018 11:15

seunlanlege changed the title ~~EIP-191/EIP-712 implementation~~ EIP-712 implementation Oct 4, 2018

ordian reviewed Oct 8, 2018

View reviewed changes

pepyakin reviewed Oct 8, 2018

View reviewed changes

util/EIP-712/src/eip712.rs Outdated Show resolved Hide resolved

util/EIP-712/src/encode.rs Outdated Show resolved Hide resolved

tomusdrw reviewed Oct 10, 2018

View reviewed changes

seunlanlege commented Oct 10, 2018

View reviewed changes

util/EIP-712/src/parser.rs Outdated Show resolved Hide resolved

seunlanlege force-pushed the EIP-712 branch 2 times, most recently from 456793c to 9d2052e Compare October 24, 2018 10:57

seunlanlege mentioned this pull request Oct 26, 2018

EIP 191 #9701

Merged

ordian reviewed Oct 29, 2018

View reviewed changes

tomusdrw approved these changes Oct 29, 2018

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

util/EIP-712/src/eip712.rs Outdated Show resolved Hide resolved

util/EIP-712/src/parser.rs Outdated Show resolved Hide resolved

seunlanlege added 3 commits October 29, 2018 23:57

EIP-712 impl

216c6b3

added more tests

9a0ce45

removed size parsing unwrap

17dad47

seunlanlege force-pushed the EIP-712 branch 2 times, most recently from ba8c0a0 to dec0bdf Compare October 29, 2018 23:40

corrected TYPE_REGEX to disallow zero sized fixed length arrays, repl…

9fe6fee

…aced LinkedHashSet with IndexSet, added API spec to docs, fixed Type::Byte encoding branch

seunlanlege force-pushed the EIP-712 branch from dec0bdf to 9fe6fee Compare October 29, 2018 23:42

seunlanlege and others added 2 commits October 30, 2018 08:22

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

838e4e2

replace .iter() with .values()

7a74b6f

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

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 30, 2018

ordian added 4 commits October 30, 2018 12:46

tabify eip712.rs

b4b9eca

use proper comments for docs

bc43cd4

Cargo.lock: revert unrelated changes

d30ec32

tabify encode.rs

701e970

ordian approved these changes Oct 30, 2018

View reviewed changes

ordian merged commit 61c1646 into master Oct 30, 2018

ordian deleted the EIP-712 branch October 30, 2018 21:12

This was referenced Nov 14, 2018

Backport Beta 2.2.1 #9905

Merged

Backport Stable 2.1.6 #9904

Merged

Tbaut added the B1-patch-beta 🕷🕷 label Nov 14, 2018

Tbaut mentioned this pull request Nov 14, 2018

Add eth_signTypedData RPC method #7462

Closed

tomusdrw mentioned this pull request Nov 15, 2018

Add experimental RPCs flag #9928

Merged

tuxxy mentioned this pull request Feb 12, 2019

Implement recently added signing standards from Parity and Geth ethereum/web3.py#1241

Closed

4 tasks

jam10o-new mentioned this pull request Nov 24, 2019

feat: Implement EIP-1024 #11284

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-712 implementation #9631

EIP-712 implementation #9631

seunlanlege commented Sep 24, 2018 •

edited

Loading

parity-cla-bot commented Sep 24, 2018

tomusdrw left a comment

tomusdrw Sep 24, 2018

tomusdrw Sep 24, 2018

5chdn commented Sep 29, 2018

seunlanlege commented Oct 1, 2018

5chdn commented Oct 1, 2018

seunlanlege commented Oct 1, 2018

tomusdrw left a comment

tomusdrw Oct 2, 2018

ordian left a comment

tomusdrw Oct 10, 2018

5chdn commented Oct 26, 2018

tomusdrw left a comment

EIP-712 implementation #9631

EIP-712 implementation #9631

Conversation

seunlanlege commented Sep 24, 2018 • edited Loading

parity-cla-bot commented Sep 24, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Sep 24, 2018

Choose a reason for hiding this comment

tomusdrw Sep 24, 2018

Choose a reason for hiding this comment

5chdn commented Sep 29, 2018

seunlanlege commented Oct 1, 2018

5chdn commented Oct 1, 2018

seunlanlege commented Oct 1, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Oct 2, 2018

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

tomusdrw Oct 10, 2018

Choose a reason for hiding this comment

5chdn commented Oct 26, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

seunlanlege commented Sep 24, 2018 •

edited

Loading