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

new ethabi #9511

Merged
merged 7 commits into from Sep 13, 2018
Merged

new ethabi #9511

merged 7 commits into from Sep 13, 2018

Conversation

debris
Copy link
Collaborator

@debris debris commented Sep 10, 2018

this pull request updates the repo to use the latest version of ethabi.

@svyatonik please carefully review changes in secret store as they were the most complicated :)

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 10, 2018
.is_document_key_store_response_required()
.call(*server_key_id, key_server.clone(), &do_call)
.unwrap_or(true)
let (encoded, decoder) = service::functions::is_server_key_generation_response_required::call(*server_key_id, *key_server);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is_server_key_generation_response_required -> is_document_key_store_response_required

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a little worrying that the tests still passed with the wrong function call. :/

@svyatonik
Copy link
Collaborator

ServiceTransactionsChecker + SS-related changes are looking good (except for 1 typo above). I'll take a look again tomorrow with a clear head (probably missed something today - not feeling good).

@5chdn 5chdn added this to the 2.1 milestone Sep 10, 2018
@@ -81,15 +81,13 @@ impl Into<trace::RewardType> for RewardKind {
#[derive(PartialEq, Debug)]
pub struct BlockRewardContract {
kind: SystemOrCodeCallKind,
block_reward_contract: block_reward_contract::BlockReward,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's not a lot left of the BlockRewardContract type. Not sure what would be better; can it become an enum perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd prefer to do it in a separate pr. It seems to be a trivial, but a relatively big change

@@ -109,7 +108,6 @@ impl URLHintContract {
/// Creates new `URLHintContract`
pub fn new(client: Arc<RegistrarClient<Call=Asynchronous>>) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

indentation is off here

@dvdplm
Copy link
Collaborator

dvdplm commented Sep 11, 2018

I am way too unfamiliar with the this code to be able to say something intelligent about it. As a general comment it seems like the new interface is slightly less ergonomic than the old (I believe the change was dictated by performance reasons though, so that's fine).
While reading I wondered if we're opening up for nasty bugs now that the contract functions are free standing functions and not tied to the type as struct members; imagine we have two contracts with identical call signatures and we happen to call the wrong one. But as said, I'm way too unfamiliar with this stuff to tell if it's a real concern or not.

@5chdn 5chdn modified the milestones: 2.1, 2.2 Sep 11, 2018
@svyatonik svyatonik added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 11, 2018
@debris
Copy link
Collaborator Author

debris commented Sep 12, 2018

While reading I wondered if we're opening up for nasty bugs now that the contract functions are free standing functions and not tied to the type as struct members;

tbh, I don't see how tying it to a struct could is any different than calling a freestanding method

imagine we have two contracts with identical call signatures and we happen to call the wrong one.

if the signatures of contract functions are the same, nothing bad would happen cause the result of such encoding would be the same.

but I guess you wanted to ask about calling two functions that have the accept params of the same type. well... there is not much we can do to prevent it... we just need to write appropriate tests for it

@dvdplm
Copy link
Collaborator

dvdplm commented Sep 13, 2018

there is not much we can do to prevent it... we just need to write appropriate tests for it

Fair enough.

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.

lgtm

@debris debris merged commit ef4a61c into master Sep 13, 2018
@debris debris deleted the new_ethabi branch September 13, 2018 09:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants