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

refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs #10657

Merged
merged 44 commits into from
Jul 3, 2019

Conversation

ltfschoen
Copy link
Contributor

@ltfschoen ltfschoen commented May 14, 2019

Purpose: Refactoring to replace untyped json! macro of #9489 with fully typed serde serialization using Rust structs for reusability and allows customization of how each field is encoded/decoded (i.e. camel case)

  • Mentioned related issue e.g. Fixes #228 or Related #1337.
  • Updated any rustdocs which may have changed
  • Implement serde json instead of json! wherever it is used apart from just TraceData
  • Replace untyped json! macro use with typed serde json TraceData in json.rs and std_json.rs. Status: resolved resolution of the Clone errors.

**Question** (solved) in json.rs i replaced use of the untyped json! macro with serde serialization, and i had to use `informant.clone()` here https://github.com//pull/10657/files#diff-98c535329ae4c9199c5b7ddd6b06b5baR85, so i had to add `Clone` to the struct here https://github.com//pull/10657/files#diff-98c535329ae4c9199c5b7ddd6b06b5baR30, and when i ran the tests they passed.

but when i did the same thing of using informant.clone() in the std_json.rs file here https://github.com/paritytech/parity-ethereum/pull/10657/files#diff-7c765744595c52ae7a64296e4264971cR206, and added Clone to the associated struct here https://github.com/paritytech/parity-ethereum/pull/10657/files#diff-7c765744595c52ae7a64296e4264971cR55, i get heaps of errors wrong number of type arguments: expected 3, found 2.
but when i add the additional first type as either Clone or clone::std:clone::Clone to address all the errors in commit 42ebcf7, when i run the tests again with cargo test -p evmbin --release -- --nocapture i get a new set of errors, and heaps of them that include: the trait std::clone::Clone cannot be made into an object, and doesn't have a size known at compile-time (which i suspect i'd resolve with lifetimes).
i could continue trying to fix those errors, but i think i've done something wrong.

  • Answer based on discussion with @tomusdrw, instead of stack: informant.clone().stack, where we clone the entire informant and have to add Informant<Clone, it could instead be informant.stack.clone(), however we can do better by not cloning the stack and storage and rather borrowing, it involves some lifetimes, but doesn't require cloning at all.

  • Fix move out of borrowed content error by cloning instance of Informant struct with Clone
  • Fix failing tests and add new
cargo test -p evmbin --release -- --nocapture

Check that it works by copying create2callPrecompiles.json and teststate.json from #10742 and run:

./target/release/parity-evm state-test ./evmbin/res/create2callPrecompiles.json --only create2callPrecompiles --chain Constantinople --json

./target/release/parity-evm state-test ./evmbin/res/teststate.json --only add12 --chain eip150 --json
  • - Remove debugging

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@ltfschoen ltfschoen added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label May 14, 2019
evmbin/src/display/json.rs Outdated Show resolved Hide resolved
@ordian
Copy link
Collaborator

ordian commented May 14, 2019

@ltfschoen I think the #9459 was already resolved by #9489, but adding more docs is welcome.

@ltfschoen ltfschoen removed the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label May 14, 2019
@ltfschoen
Copy link
Contributor Author

Closing as this task has already been completed!

@ltfschoen ltfschoen closed this May 14, 2019
@sorpaas
Copy link
Collaborator

sorpaas commented May 14, 2019

@ordian i still think some of this PR can be useful -- replacing macro with fully typed serialization is a little bit better IMO. My opinion is that, not only we may be using it somewhere else in the future, but also, it allows us to customize how each field are encoded/decoded (camel case!) so it's a little bit more rusty. :)

WDYT?

@ordian
Copy link
Collaborator

ordian commented May 14, 2019

@sorpaas I agree the refactoring to use rust structs is better than untyped json!, though I think the original issue was resolved. Sorry I didn't mean we should close this PR.

@ordian ordian reopened this May 14, 2019
@ltfschoen ltfschoen changed the title refactor: Related #9459 - evmbin: replace hardcoded json output logic with serde serialization refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs May 14, 2019
@ltfschoen ltfschoen added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label May 14, 2019
@ordian ordian added this to the 2.6 milestone Jun 17, 2019
ltfschoen and others added 2 commits June 18, 2019 13:23
Co-Authored-By: Andronik Ordian <write@reusable.software>
@ltfschoen ltfschoen added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 19, 2019
evmbin/src/display/json.rs Outdated Show resolved Hide resolved
evmbin/src/display/json.rs Outdated Show resolved Hide resolved
evmbin/src/display/std_json.rs Outdated Show resolved Hide resolved
evmbin/src/display/std_json.rs Outdated Show resolved Hide resolved
evmbin/src/display/std_json.rs Outdated Show resolved Hide resolved
evmbin/src/display/std_json.rs Outdated Show resolved Hide resolved
evmbin/src/display/std_json.rs Outdated Show resolved Hide resolved
ltfschoen and others added 11 commits June 20, 2019 07:13
…_gas success

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

Co-Authored-By: Andronik Ordian <write@reusable.software>
… finish for state root

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

Co-Authored-By: Andronik Ordian <write@reusable.software>
…r state root

Co-Authored-By: Andronik Ordian <write@reusable.software>
…r finish success

Co-Authored-By: Andronik Ordian <write@reusable.software>
…r finish failure

Co-Authored-By: Andronik Ordian <write@reusable.software>
@ltfschoen ltfschoen requested a review from dvdplm June 27, 2019 16:25
@ordian
Copy link
Collaborator

ordian commented Jul 2, 2019

@dvdplm could you take a look again?

Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

minor grumble

accounts/ethstore/src/json/crypto.rs Outdated Show resolved Hide resolved
Co-Authored-By: David <dvdplm@gmail.com>
@dvdplm dvdplm merged commit 02e33c4 into master Jul 3, 2019
dvdplm added a commit that referenced this pull request Jul 4, 2019
…me-parent

* master:
  refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812)
  Break circular dependency between Client and Engine (part 1) (#10833)
  tests: Relates to #10655: Test instructions for Readme (#10835)
  refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
dvdplm added a commit that referenced this pull request Jul 4, 2019
* master:
  refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812)
  Break circular dependency between Client and Engine (part 1) (#10833)
  tests: Relates to #10655: Test instructions for Readme (#10835)
  refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
  cargo update -p smallvec (#10822)
  replace memzero with zeroize crate (#10816)
  Don't repeat the logic from Default impl (#10813)
  removed additional_params method (#10818)
  Add Constantinople eips to the dev (instant_seal) config (#10809)
dvdplm added a commit that referenced this pull request Jul 4, 2019
* master: (21 commits)
  refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812)
  Break circular dependency between Client and Engine (part 1) (#10833)
  tests: Relates to #10655: Test instructions for Readme (#10835)
  refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
  cargo update -p smallvec (#10822)
  replace memzero with zeroize crate (#10816)
  Don't repeat the logic from Default impl (#10813)
  removed additional_params method (#10818)
  Add Constantinople eips to the dev (instant_seal) config (#10809)
  removed redundant fmt::Display implementations (#10806)
  revert changes to .gitlab-ci.yml (#10807)
  Add filtering capability to `parity_pendingTransactions` (issue 8269) (#10506)
  ...
dvdplm added a commit that referenced this pull request Jul 4, 2019
* master:
  Extricate PodAccount and state Account to own crates (#10838)
  logs (#10817)
  refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812)
  Break circular dependency between Client and Engine (part 1) (#10833)
  tests: Relates to #10655: Test instructions for Readme (#10835)
  refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)
dvdplm added a commit that referenced this pull request Jul 4, 2019
…hore/extricate-state-backend

* dp/chore/extricate-account-db-into-own-crate:
  third time's the charm
  test failure 2
  test failure
  Extricate PodAccount and state Account to own crates (#10838)
  logs (#10817)
  refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812)
  Break circular dependency between Client and Engine (part 1) (#10833)
  tests: Relates to #10655: Test instructions for Readme (#10835)
  refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)
@niklasad1 niklasad1 deleted the luke-9459-evmbin-serde branch November 12, 2019 20:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants