-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ethabi version 5 #7723
ethabi version 5 #7723
Conversation
updater/src/lib.rs
Outdated
@@ -23,16 +23,16 @@ extern crate parking_lot; | |||
extern crate parity_hash_fetch as hash_fetch; | |||
extern crate ethcore; | |||
extern crate ethabi; | |||
#[macro_use] extern crate ethabi_derive; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to have a separate block of #[macro_use]
extern crate statements.
updater/src/updater.rs
Outdated
@@ -161,21 +176,34 @@ impl Updater { | |||
} | |||
|
|||
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is &
needed?
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 |
updater/src/types/release_track.rs
Outdated
|
||
impl From<ReleaseTrack> for u8 { | ||
fn from(rt: ReleaseTrack) -> Self { | ||
match rt { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just rt as u8
instead of that match
?
@@ -136,8 +136,8 @@ fn make_chain(accounts: Arc<AccountProvider>, blocks_beyond: usize, transitions: | |||
vec![transaction] | |||
}; | |||
|
|||
let contract_1 = ValidatorSet::new(*CONTRACT_ADDR_1); | |||
let contract_2 = ValidatorSet::new(*CONTRACT_ADDR_2); | |||
let contract_1 = test_validator_set::ValidatorSet::default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can be wrong, but looks like both contract_1
and contract_2
can now be replaced with single contract
variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are absolutely right!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I will give a fresh look on updater logic tomorrow morning.
@@ -788,7 +789,7 @@ mod tests { | |||
header.set_parent_hash(parent_hash); | |||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps change the loop range to be u32
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also header.set_number(i)
which uses i
. So one of them have to be casted
ethcore/node_filter/src/lib.rs
Outdated
extern crate parking_lot; | ||
extern crate lru_cache; | ||
|
||
extern crate ethabi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in previous section.
ethcore/node_filter/src/lib.rs
Outdated
#[macro_use] | ||
extern crate ethabi_derive; | ||
#[macro_use] | ||
extern crate ethabi_contract; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not. it exposes use_contract!
macro which is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I mean the new line :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk :)
ethcore/node_filter/src/lib.rs
Outdated
use native_contracts::PeerSet as Contract; | ||
use network::{NodeId, ConnectionFilter, ConnectionDirection}; | ||
use lru_cache::LruCache; | ||
use parking_lot::Mutex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these two are in the same section as std
? IMHO it should be
std-imports
external-imports
internal-imports
all sorted.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You sure, you need both clone()
and .as_ref()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely not, that's just sloppy refactoring :p
|
||
Some(Contract::new(contract_addr)) | ||
}) | ||
let mut contract_address = self.contract_address.lock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
} | ||
|
||
/// 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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug asserts
are useless, just use regular assert
or remove
secret_store/src/key_server_set.rs
Outdated
@@ -299,14 +357,13 @@ impl CachedContract { | |||
} | |||
|
|||
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change in the logic. It keeps the old address even if you pass None
as new_contract_address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch!
secret_store/src/lib.rs
Outdated
extern crate keccak_hash as hash; | ||
extern crate kvdb; | ||
extern crate kvdb_rocksdb; | ||
|
||
extern crate ethabi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be joined with other imports imho (and sorted)
updater/src/lib.rs
Outdated
@@ -16,23 +16,26 @@ | |||
|
|||
//! Updater for Parity executables | |||
|
|||
#[macro_use] extern crate log; | |||
#[macro_use] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually have a convention of having separate section for macro_use
extern crates.
@tomusdrw thanks for review! Can you take a look at the pr again? 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've found a serious issue in updater logic (introduced after refactoring). Would be good to have some tests for this.
#[cfg(test)] | ||
extern crate kvdb_memorydb; | ||
#[macro_use] | ||
extern crate log; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed #[cfg(test)]
with #[macro_use]
, but it's a minor issue.
|
||
let latest_binary = self.operations_contract.functions() | ||
.checksum() | ||
.call(client_id_hash(), release_id, platform_id_hash(), &do_call) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
updater/src/updater.rs
Outdated
|
||
// 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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
updater/src/updater.rs
Outdated
_ => { in_minor = None; break; } | ||
}; | ||
|
||
let latest_in_track = self.operations_contract.functions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we enclose this into a function? It's the second time we write exactly the same 4 lines
updater/src/updater.rs
Outdated
}) | ||
} else { | ||
Err("Operations not available".into()) | ||
in_minor = Some(self.collect_release_info(latest_in_track)?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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),
};
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
fix build ```version``` after #7723
fix build ```version``` after #7723
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
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:
5.x
native_contracts
crate. we useethabi_contract
andethabi_derive
instead. Now creating contract interface has no runtime costethereum-types
to version0.2
. fixed cases whenx.hex()
was used instead offormat!("{:x}", x)