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

Refactor updater to use ethabi-derive#6896

Closed
axelchalon wants to merge 3 commits into
masterfrom
refactor-updater-ethabi-derive
Closed

Refactor updater to use ethabi-derive#6896
axelchalon wants to merge 3 commits into
masterfrom
refactor-updater-ethabi-derive

Conversation

@axelchalon
Copy link
Copy Markdown
Contributor

@axelchalon axelchalon commented Oct 25, 2017

Use ethabi-derive (with call function @ this branch: https://github.com/paritytech/ethabi/pull/62) to generate the updater contract, rather than using the auto-generated operations.rs contract file. The ABI passed to use_contract! is the same as before.

Todo:

  • Needs testing! @lght is currently writing tests on the updater based on master Updater unit tests #6913 ; we'll want to make sure they also pass on this branch.

Before merging:

Issues/questions:

  • Right now ethabi-derive call returns an ethabi::Result (i.e. Result<_, ethabi::Error>). However, updater.rs expects Result<_, String>, so I need to map_err the ethabi::Error into a string each time (when using ? for example); not sure if this is optimal. Let me know if this should be done another way; I imagine this will change with the error-chain refactoring by Marek.
  • Why is collect_release_info a static method? I kept it this way but I'm wondering why it cannot be made a regular method so we don't have to pass the contract and the call method to it.
  • In the previous version (master), the generated contract had specific input/output types based on the ABI (e.g. (u32, u8, u32, bool)). ethabi-derive currently uses pretty generic input/output types (e.g. (ethabi::Uint , ethabi::Uint , ethabi::Uint , bool)). This creates the need for quite a bit of boilerplate to convert the types to and fro (I imagine this will also be linked with Marek's refactoring for unifying the types). Same for strings; they need to be converted to a hash before being passed as an input. Wouldn't it better if the functions generated by ethabi-derive took care of this and matched the function signatures in the ABI more closely?

This change is Reviewable

@axelchalon axelchalon added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 25, 2017
@5chdn 5chdn added this to the 1.9 milestone Oct 30, 2017
@lght lght added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 30, 2017
Copy link
Copy Markdown

@lght lght left a comment

Choose a reason for hiding this comment

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

Couple of minor grumbles about dependency checksums and abstracting do_call's type. Other than that, the changes look great!

Comment thread updater/src/updater.rs Outdated
fetcher: Mutex<Option<fetch::Client>>,
operations: Mutex<Option<Operations>>,
operations_contract: operations_contract::Operations,
do_call: Mutex<Option<Box<Fn(Vec<u8>) -> Result<Vec<u8>, String> + Send + Sync + 'static>>>,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you abstract do_call's type with a type alias? Maybe above UpdateFilter you could add:
pub type DoCallFn = Fn(Vec<u8>) -> Result<Vec<u8>, String> + Send + Sync + 'static>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

Comment thread Cargo.lock
"checksum ethabi 4.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "c819a3adef0413a2519cbd9a19a35dd1c20c7a0110705beaba8aa4aa87eda95f"
"checksum ethabi 4.1.0 (git+https://github.com/paritytech/ethabi/?branch=call-function)" = "<none>"
"checksum ethabi-contract 4.1.0 (git+https://github.com/paritytech/ethabi/?branch=call-function)" = "<none>"
"checksum ethabi-derive 4.1.0 (git+https://github.com/paritytech/ethabi/?branch=call-function)" = "<none>"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will checksums automatically get added to the ethabi dependencies? I'm not very familiar with how checksums are calculated/stored for github dependencies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know, I'm not familiar with them either, but we will switch to a regular dependency (on crates.io) once the ethabi branch is merged; this will probably add the checksums again?

Copy link
Copy Markdown

@lght lght Nov 2, 2017

Choose a reason for hiding this comment

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

Yeah, pretty sure that crates.io includes the checksum with the crate. If this will be pulling from crates.io on merge, I have no issues with the missing checksum here. Think cargo pins the a release hash to the github url anyway, so secure enough for dev purposes. Just didn't want it to get rolled into master that way

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm waiting for https://github.com/paritytech/ethabi/pull/62 to be merged and then I'll update the dependency in this PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ok sounds good. Everything else looks great. So far from the tests I've written, your changes are easy to incorporate.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've added paritytech/core-devs to the owners list of ethabi_derive. I've also released 4.2, so feel free to use it instead of github repo ;)

@lght lght added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 2, 2017
@5chdn
Copy link
Copy Markdown
Contributor

5chdn commented Nov 10, 2017

A0 and A3 labels are clearly conflicting :)

@axelchalon
Copy link
Copy Markdown
Contributor Author

I don't plan to make modifications to the code, so reviewing can happen now, but I need to wait for the tests #6913 to be written until this can be merged ^^ I guess I'll ask for a review once this PR is ready to get merged :)

@axelchalon axelchalon removed the A0-pleasereview 🤓 Pull request needs code review. label Nov 10, 2017
@tomusdrw tomusdrw added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 21, 2017
@tomusdrw
Copy link
Copy Markdown
Collaborator

On ice, waiting for #6913

@5chdn 5chdn modified the milestones: 1.9, 1.10 Dec 30, 2017
@5chdn 5chdn added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Jan 9, 2018
@lght lght mentioned this pull request Jan 15, 2018
@5chdn 5chdn modified the milestones: 1.10, 1.11 Jan 23, 2018
@debris
Copy link
Copy Markdown
Collaborator

debris commented Jan 25, 2018

I'll take it from here

@debris debris mentioned this pull request Jan 29, 2018
@debris debris closed this Jan 29, 2018
@5chdn 5chdn deleted the refactor-updater-ethabi-derive branch January 30, 2018 11:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. M4-core ⛓ Core client code / Rust.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants