Correct EIP-712 encoding #11092

seunlanlege · 2019-09-26T08:23:25Z

Previously when building a type hash for a given struct, the build_dependencies function was unaware that structs could have fields that are arrays of custom types e.g

struct Person {
    address[] wallets
    string name
}

struct Group {
    string name
    Person[] members
}

so when it tried to encode the Group type it produced an incorrect encoding

Group(string name, Person[] members)

instead of:

Group(string name, Person[] members)Person(string name, address[] wallets)

which caused: #11087

this PR addresses this issue and closes #11087

dvdplm

Looks close to the JS impl, which is good.

Unrelated q: this crate uses lunarity 0.1. Is there a point in upgrading to 0.2?

util/EIP-712/src/encode.rs

seunlanlege · 2019-09-26T09:46:26Z

Unrelated q: this crate uses lunarity 0.1. Is there a point in upgrading to 0.2?

lunarity 0.2 does bump up performance

util/EIP-712/src/encode.rs

dvdplm · 2019-09-26T10:20:23Z

lunarity 0.2 does bump up performance

Can you add a ticket for it please? :)

seunlanlege · 2019-09-26T10:57:17Z

already bumped it

seunlanlege · 2019-09-27T07:49:57Z

rpc failing tests strike again.
@niklasad1 could you help restart CI

niklasad1 · 2019-09-27T09:50:27Z

@seunlanlege done, but in the future, you can do it by yourself by just logging in to gitlab with your github account. Otherwise, ping the CI-team and they can help you :)

Co-Authored-By: Andronik Ordian <write@reusable.software>

util/EIP-712/src/encode.rs

niklasad1 · 2019-09-30T08:57:52Z

util/EIP-712/src/parser.rs

 	}
+
+	Ok(token.ok_or_else(|| ErrorKind::NonExistentType)?)


could be ok_or instead because it is just a value.

niklasad1

LGTM, feel free to merge this.

I added a couple of inline comments that are optional.

Also, the code uses return Err(e)? which https://rust-lang.github.io/rust-clippy/master/index.html#try_err doesn't like but I don't have any strong opinions on it seems fine.

* support encoding custom array types as fields * new line * removed expect * Update util/EIP-712/src/encode.rs Co-Authored-By: Andronik Ordian <write@reusable.software> * bump lunarity * update cargo lock * nits * nits

* ropsten #6631425 foundation #8798209 (#11201) * [stable] builtin, istanbul and mordor testnet backports (#11234) * ethcore-builtin (#10850) * [builtin]: support `multiple prices and activations` in chain spec (#11039) * [chain specs]: activate `Istanbul` on mainnet (#11228) * ethcore/res: add mordor testnet configuration (#11200) * Update list of bootnodes for xDai chain (#11236) * ethcore: remove `test-helper feat` from build (#11047) * Secret store: fix Instant::now() related race in net_keep_alive (#11155) (#11159) * [stable]: backport #10691 and #10683 (#11143) * Fix compiler warning (that will become an error) (#10683) * Refactor Clique stepping (#10691) * Add Constantinople eips to the dev (instant_seal) config (#10809) * Add cargo-remote dir to .gitignore (?) * Insert explicit warning into the panic hook (#11225) * Fix docker centos build (#11226) * Update MIX bootnodes. (#11203) * Use provided usd-per-eth value if an endpoint is specified (#11209) * Add new line after writing block to hex file. (#10984) * Type annotation for next_key() matching of json filter options (#11192) (but no `FilterOption` in 2.5 so…) * Upgrade jsonrpc to latest (#11206) * [CI] check evmbin build (#11096) * Correct EIP-712 encoding (#11092) * [client]: Fix for incorrectly dropped consensus messages (#11086) * Fix block detail updating (#11015) * Switching sccache from local to Redis (#10971) * Made ecrecover implementation trait public (#11188) * [dependencies]: jsonrpc `14.0.1` (#11183) * [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179) * [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) * util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) * ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) * Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) * Cleanup stratum a bit (#11161) * Upgrade to jsonrpc v14 (#11151) * SecretStore: expose restore_key_public in HTTP API (#10241)

* [CI] check evmbin build (#11096) * Correct EIP-712 encoding (#11092) * [client]: Fix for incorrectly dropped consensus messages (#11082) (#11086) * Update hardcoded headers (foundation, classic, kovan, xdai, ewc, ...) (#11053) * Add cargo-remote dir to .gitignore (?) * Update light client headers: ropsten 6631425 foundation 8798209 (#11201) * Update list of bootnodes for xDai chain (#11236) * ethcore/res: add mordor testnet configuration (#11200) * [chain specs]: activate Istanbul on mainnet (#11228) * [builtin]: support multiple prices and activations in chain spec (#11039) * [receipt]: add sender & receiver to RichReceipts (#11179) * [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) * Made ecrecover implementation trait public (#11188) * Fix docker centos build (#11226) * Update MIX bootnodes. (#11203) * Insert explicit warning into the panic hook (#11225) * Use provided usd-per-eth value if an endpoint is specified (#11209) * Cleanup stratum a bit (#11161) * Add Constantinople EIPs to the dev (instant_seal) config (#10809) (already backported) * util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) * ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) * Type annotation for next_key() matching of json filter options (#11192) * Upgrade jsonrpc to latest (#11206) * [dependencies]: jsonrpc 14.0.1 (#11183) * Upgrade to jsonrpc v14 (#11151) * Switching sccache from local to Redis (#10971) * Snapshot restoration overhaul (#11219) * Add new line after writing block to hex file. (#10984) * Pause pruning while snapshotting (#11178) * Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) * Fix block detail updating (#11015) * Make InstantSeal Instant again #11186 * Filter out some bad ropsten warp snapshots (#11247)

seunlanlege added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Sep 26, 2019

dvdplm approved these changes Sep 26, 2019

View reviewed changes

dvdplm added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Sep 26, 2019

ordian reviewed Sep 26, 2019

View reviewed changes

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

ordian added this to the 2.7 milestone Sep 26, 2019

ordian added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 26, 2019

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 26, 2019

ordian approved these changes Sep 26, 2019

View reviewed changes

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

dvdplm added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Sep 26, 2019

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Sep 27, 2019

ordian added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Sep 27, 2019

seunlanlege and others added 5 commits September 30, 2019 09:00

support encoding custom array types as fields

b9b4c05

new line

255b5ed

removed expect

a7b4bf3

Update util/EIP-712/src/encode.rs

4c882d1

Co-Authored-By: Andronik Ordian <write@reusable.software>

bump lunarity

ca045b2

seunlanlege force-pushed the seun-fix-eip-712-encoding branch from 1a73648 to ca045b2 Compare September 30, 2019 08:01

update cargo lock

fee3b84

niklasad1 added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Sep 30, 2019

niklasad1 reviewed Sep 30, 2019

View reviewed changes

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

niklasad1 reviewed Sep 30, 2019

View reviewed changes

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Sep 30, 2019

niklasad1 approved these changes Sep 30, 2019

View reviewed changes

seunlanlege added 2 commits September 30, 2019 16:23

nits

009f54d

nits

e8955e8

seunlanlege merged commit 8471b91 into master Sep 30, 2019

ordian deleted the seun-fix-eip-712-encoding branch September 30, 2019 21:17

This was referenced Nov 5, 2019

v2.6.5 beta #11240

Merged

v2.5.10 stable #11239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct EIP-712 encoding #11092

Correct EIP-712 encoding #11092

seunlanlege commented Sep 26, 2019

dvdplm left a comment

seunlanlege commented Sep 26, 2019

dvdplm commented Sep 26, 2019

seunlanlege commented Sep 26, 2019

seunlanlege commented Sep 27, 2019 •

edited

Loading

niklasad1 commented Sep 27, 2019

niklasad1 Sep 30, 2019

niklasad1 left a comment •

edited

Loading

Correct EIP-712 encoding #11092

Correct EIP-712 encoding #11092

Conversation

seunlanlege commented Sep 26, 2019

dvdplm left a comment

Choose a reason for hiding this comment

seunlanlege commented Sep 26, 2019

dvdplm commented Sep 26, 2019

seunlanlege commented Sep 26, 2019

seunlanlege commented Sep 27, 2019 • edited Loading

niklasad1 commented Sep 27, 2019

niklasad1 Sep 30, 2019

Choose a reason for hiding this comment

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

seunlanlege commented Sep 27, 2019 •

edited

Loading

niklasad1 left a comment •

edited

Loading