ethabi version 5 by debris · Pull Request #7723 · openethereum/parity-ethereum

debris · 2018-01-29T18:11:04Z

replaces #6896

the real number of lines changed is: +1,053 -2,097. 560 added lines belong to a single json file which was inlined before

changes:

bumped ethabi to version 5.x
fixed Retain cycle in updater #7720 - retain cycle in updater
closes Migrate to ethabi 4.0.0 #6360
removed native_contracts crate. we use ethabi_contract and ethabi_derive instead. Now creating contract interface has no runtime cost
fixes updates when updater is configured to track minor version
bumped ethereum-types to version 0.2. fixed cases when x.hex() was used instead of format!("{:x}", x)
updated several libraries to their latest version to resolve dependencies conflicts

#7720

…terface

tomusdrw · 2018-01-29T18:23:40Z

 extern crate parity_hash_fetch as hash_fetch;
 extern crate ethcore;
 extern crate ethabi;
+#[macro_use] extern crate ethabi_derive;


I'd prefer to have a separate block of #[macro_use] extern crate statements.

tomusdrw · 2018-01-30T14:05:25Z


 	fn collect_latest(&self) -> Result<OperationsInfo, String> {
-		if let Some(ref operations) = *self.operations.lock() {
+		if let &Some(ref do_call) = &*self.do_call.lock() {


Why is & needed?

…n on update

debris · 2018-02-06T17:13:07Z

updater needs much more work, but I for now, I've decided to not modify it's core logic in this pr.

I'll create a separate pr with tests and later I'll refactor it

svyatonik · 2018-02-07T07:48:01Z

+
+impl From<ReleaseTrack> for u8 {
+	fn from(rt: ReleaseTrack) -> Self {
+		match rt {


Maybe just rt as u8 instead of that match?

svyatonik · 2018-02-07T07:54:24Z


-		let contract_1 = ValidatorSet::new(*CONTRACT_ADDR_1);
-		let contract_2 = ValidatorSet::new(*CONTRACT_ADDR_2);
+		let contract_1 = test_validator_set::ValidatorSet::default();


I can be wrong, but looks like both contract_1 and contract_2 can now be replaced with single contract variable.

you are absolutely right!

svyatonik

SS changes are OK. Overall also seems fine (separating contract and its address is the most awaited [at least by me] feature :) ). Would be great to have another review - I'm not so familiar with updater and other contracts.

…ts for it

tomusdrw

Looks good. I will give a fresh look on updater logic tomorrow morning.

tomusdrw · 2018-02-07T18:03:09Z

 			header.set_number(i);
 			header.set_timestamp(rolling_timestamp);
-			header.set_difficulty(*genesis_header.difficulty() * i.into());
+			header.set_difficulty(*genesis_header.difficulty() * i as u32);


perhaps change the loop range to be u32 instead?

there is also header.set_number(i) which uses i. So one of them have to be casted

tomusdrw · 2018-02-07T18:04:08Z

 extern crate parking_lot;
+extern crate lru_cache;
+
+extern crate ethabi;


This should be in previous section.

tomusdrw · 2018-02-07T18:04:15Z

+extern crate ethabi_derive;
+#[macro_use]
+extern crate ethabi_contract;



it's not. it exposes use_contract! macro which is used

(I mean the new line :))

tomusdrw · 2018-02-07T18:05:36Z

-use native_contracts::PeerSet as Contract;
-use network::{NodeId, ConnectionFilter, ConnectionDirection};
+use lru_cache::LruCache;
+use parking_lot::Mutex;


Why these two are in the same section as std? IMHO it should be

std-imports external-imports internal-imports

all sorted.

tomusdrw · 2018-02-07T18:10:45Z

-		let client = self.client.read().clone();
-		Box::new(move |a, d| client.as_ref()
+	fn transact(&self, data: Bytes) -> Result<(), String> {
+		let client = self.client.read().clone().as_ref()


You sure, you need both clone() and .as_ref()?

you are right ;)

definitely not, that's just sloppy refactoring :p

tomusdrw · 2018-02-07T18:32:53Z

-
-					Some(Contract::new(contract_addr))
-				})
+		let mut contract_address = self.contract_address.lock();


The whole method is useless imho. just call registrar on check every time (since the address of the contract may change in the meantime) or call once at the start and memoize if we don't care that it might change.

With this approach we only support a case where the contract address is set to the registry, but we started earlier. IMHO it's a weird / too specific.

tomusdrw · 2018-02-07T18:33:21Z

 	/// Checks if service transaction can be appended to the transaction queue.
 	pub fn check(&self, client: &ContractCaller, tx: &SignedTransaction) -> Result<bool, String> {
-		debug_assert_eq!(tx.gas_price, U256::zero());
+		debug_assert!(tx.gas_price.is_zero());


debug asserts are useless, just use regular assert or remove

tomusdrw · 2018-02-07T18:39:09Z


 	fn read_from_registry(&mut self, client: &Client, new_contract_address: Option<Address>) {
-		self.contract = new_contract_address.map(|contract_addr| {
+		if let Some(contract_addr) = new_contract_address {


This is a change in the logic. It keeps the old address even if you pass None as new_contract_address

great catch!

tomusdrw · 2018-02-07T18:41:14Z

 extern crate kvdb;
 extern crate kvdb_rocksdb;

+extern crate ethabi;


Should be joined with other imports imho (and sorted)

tomusdrw · 2018-02-07T18:42:56Z

 //! Updater for Parity executables

-#[macro_use] extern crate log;
+#[macro_use]


We usually have a convention of having separate section for macro_use extern crates.

debris · 2018-02-08T11:02:14Z

@tomusdrw thanks for review! Can you take a look at the pr again? 😉

tomusdrw

I think I've found a serious issue in updater logic (introduced after refactoring). Would be good to have some tests for this.

tomusdrw · 2018-02-08T13:31:54Z

+#[cfg(test)]
+extern crate kvdb_memorydb;
+#[macro_use]
+extern crate log;


Mixed #[cfg(test)] with #[macro_use], but it's a minor issue.

tomusdrw · 2018-02-08T13:39:09Z

+
+		let latest_binary = self.operations_contract.functions()
+			.checksum()
+			.call(client_id_hash(), release_id, platform_id_hash(), &do_call)


Why is it passing hashes now? Were client_id and platform hashed internally in the checksum function previously?
Checked the function and it doesn't seem so.

EDIT: *_hash() functions are misleading - I thought it changed the logic and we now pass hashed data, perhaps it should be called client_id_h256() or client_id_as_h256() to avoid confusion.

tomusdrw · 2018-02-08T13:48:12Z

+
+		// if the minor version has changed, let's check the minor version on a different track
+		while in_minor.as_ref().expect(PROOF).version.version.minor != self.this.version.minor {
+			let track = match self.track() {


This seems to be invalid (and different from previous behaviour). Afaict it may end up in an endless loop (since we always use self.track() instead of in_minor.track)

tomusdrw · 2018-02-08T13:49:37Z

+				_ => { in_minor = None; break; }
+			};
+
+			let latest_in_track = self.operations_contract.functions()


Can we enclose this into a function? It's the second time we write exactly the same 4 lines

tomusdrw · 2018-02-08T14:03:32Z

-			})
-		} else {
-			Err("Operations not available".into())
+			in_minor = Some(self.collect_release_info(latest_in_track)?);


Imho would be cleaner to use loop instead of while (and enclose the whole thing in a function).

let (in_track, in_minor) = self.find_versions(self.track()); // impl fn find_versions(&self, current_track: _) -> (_, Option<_>) { let mut version_in_track = None; let mut track = current_track; loop { let latest_in_track = ...; let in_minor = self.collect_release_info(latest_in_track)?; // update initial version_in_track version_in_track = version_in_track.or_else(|| Some(in_minor.clone())); // check version if in_minor.version.version.minor == self.this.version.minor { return (version_in_track.unwrap(), Some(in_minor)); } // check next track track = match in_minor.track { ReleaseTrack::Beta => ReleaseTrack::Stable, ReleaseTrack::Nightly => ReleaseTrack::Beta, _ => return (version_in_track.unwrap(), None), }; } }

One more idea:

let in_track = self.find_version(self.track()); let in_minor = self.find_minor_version(in_track); // impl fn find_minor_version(&self, start_track: _) -> Option<_> { let mut track = start_track; while track.version.version.minor != self.this.version.minor { let next_track = match track.track { ReleaseTrack::Beta => ReleaseTrack::Stable, ReleaseTrack::Nightly => ReleaseTrack::Beta, _ => return None, }; track = self.find_version(next_track); } return Some(track); } fn find_version(track: _) -> _ { let release_info = ...; self.collect_release_info(release_info)?; }

fix build ```version``` after #7723

* Resolve conflicts * Resolve conflicts * Update gitlab-build.sh (#7855) fix build ```version``` after #7723 * Resolve conflicts * Update gitlab-test.sh (#7883) * Update gitlab-test.sh #7871 * Update aura-test.sh * Backport master ci prs * Restore js-build * Bump stable to 1.8.10 * Bump stable to 1.8.10 * Make track stable

* Resolve conflicts * Resolve conflicts * Update gitlab-build.sh (#7855) fix build ```version``` after #7723 * Resolve conflicts * Update gitlab-test.sh (#7883) * Update gitlab-test.sh #7871 * Update aura-test.sh * Backport master ci prs * Bump beta to 1.9.3 * Make track beta * Downgrade rustc_version

axelchalon and others added 10 commits October 25, 2017 15:33

Refactor updater to use ethabi-derive

4512701

Grumble: do_call type alias

6f583c5

Empty commit to trigger test re-run

a970c45

migration to ethabi-5.0

9bf5943

Merge branch 'master' into ethabi-5

109dc3f

migration to ethabi-5.0 in progress

4e6bbf3

ethabi 5.1

6f6800e

use ethabi_deriven to generate TransactAcl contract

6d5b9aa

use ethabi_deriven to generate Registry contract

e51acbe

hash-fetch uses ethabi_derive, removed retain cycle from updater, fixed

7b0c5ec

#7720

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

debris added 2 commits January 29, 2018 17:02

node-filter crate uses ethabi_derive to generate peer_set contract in…

867016b

…terface

use LruCache in node-filter instead of HashMap

66a0076

5chdn added this to the 1.10 milestone Jan 30, 2018

tomusdrw reviewed Jan 30, 2018

View reviewed changes

debris added 14 commits January 31, 2018 12:53

Merge branch 'master' into ethabi-5

8fb80d1

validator_set engine uses ethabi_derive

f435922

ethcore does not depend on native_contracts

816eb13

miner does no depend on native_contracts

2f26dd9

secret_store does not use native_contracts (in progress)

0dc5d57

removed native-contracts

dc84c4e

ethcore and updater does not depend on futures

6a9e653

updated ethereum-types

7e3b077

Merge branch 'master' into ethabi-5

fbd0c7e

fixed all warnings caused by using new version of ethereum-types

bf6cbb0

Merge branch 'master' into ethabi-5

8f55177

updated ethabi_derive && ethabi_contract to get rid of warnings

86437e8

removed another retain cycle in updater, fixed following minor versio…

1b44389

…n on update

moved contracts out of native_contracts res

f207aa0

debris requested a review from svyatonik February 6, 2018 17:19

debris added 2 commits February 6, 2018 19:42

fixed failing test

dde4083

fixed failing test

0704701

svyatonik reviewed Feb 7, 2018

View reviewed changes

svyatonik approved these changes Feb 7, 2018

View reviewed changes

debris added 3 commits February 7, 2018 09:46

Merge branch 'master' into ethabi-5

406b03f

there is no need to create two contracts of the same kind any more

29321d5

simplify updater::ReleaseTrack conversion into u8 and add several tes…

2ac0a88

…ts for it

This was referenced Feb 7, 2018

remove registrar logic from the Client #7821

Closed

lazy evaluation of registrar address #7822

Closed

Merge branch 'master' into ethabi-5

74ee624

tomusdrw reviewed Feb 7, 2018

View reviewed changes

applied review suggestions

59e13a3

tomusdrw reviewed Feb 8, 2018

View reviewed changes

applied review suggestions

ac15ce2

tomusdrw approved these changes Feb 9, 2018

View reviewed changes

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

debris merged commit c060d95 into master Feb 9, 2018

debris deleted the ethabi-5 branch February 9, 2018 08:32

General-Beck added a commit that referenced this pull request Feb 10, 2018

Update gitlab-build.sh

c09bb1c

fix build ```version``` after #7723

General-Beck mentioned this pull request Feb 10, 2018

Update gitlab-build.sh #7855

Merged

debris pushed a commit that referenced this pull request Feb 12, 2018

Update gitlab-build.sh (#7855)

78f39e9

fix build ```version``` after #7723

5chdn pushed a commit that referenced this pull request Feb 14, 2018

Update gitlab-build.sh (#7855)

b8ec7d5

fix build ```version``` after #7723

5chdn pushed a commit that referenced this pull request Feb 14, 2018

Update gitlab-build.sh (#7855)

8c46864

fix build ```version``` after #7723

Conversation

debris commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

debris commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svyatonik left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

debris Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

debris commented Feb 8, 2018

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

debris commented Jan 29, 2018 •

edited

Loading

debris Feb 8, 2018 •

edited

Loading