[contract-client] refactor #7978

niklasad1 · 2018-02-22T13:07:38Z

Hi,
This is a step toward closing #7821 but after realizing that it requires major refactoring in ethcore we decided to skip that because it also dependent on #7038.

Thus, the PR provides the following:

A new crate contract-client that is meant handle interactions to the registrar contract instead of having multiple interfaces and moves this functionality from hash-fetch

parity-cla-bot · 2018-02-22T13:07:40Z

It looks like @niklasad1 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

rphmeier · 2018-02-22T16:17:55Z

contract_client/src/contract_client.rs

+use_contract!(registry, "Registry", "res/registrar.json");
+
+// This need to be documented why it is concatenated with the hash
+const MAGIC_CONST: &'static str = "A";


I think it's meant to be an "A" record as in DNS

rphmeier · 2018-02-22T16:19:06Z

contract_client/src/contract_client.rs

+
+/// Syncronous contract interface.
+/// Should execute transaction using current blockchain state.
+pub trait ContractClient: Send + Sync {


I don't see the reason for the split between the traits.
Add a type Call: IntoFuture<Item=Bytes,Error=String>.

One implementation can use Result and another uses Box<Future>

and is this trait actually used anywhere?

No, it is not used anywhere but ideally this should be used ethcore which is synchronous if I'm not mistaken!

rphmeier · 2018-02-22T16:32:53Z

synchronous (to be used by LightClient)

asyncronous (to be used by everyhing else)

This seems backwards. The light client by nature serves contract call requests asynchronously (as it has to go to the network), while full nodes don't even have a futures-based API for doing so.

rphmeier · 2018-02-22T16:35:37Z

contract-client as a crate name is way too general and not a good description of its contents. Maybe registrar-wrapper or something which describes its intended use.

niklasad1 · 2018-02-23T15:53:11Z

This seems backwards. The light client by nature serves contract call requests asynchronously (as it has to go to the network), while full nodes don't even have a futures-based API for doing so.

Ok, maybe I misunderstood Marek then but at least the API that ethcore/miner is using is synchronous by only looking at the method signatures. Maybe that requires another trait for later but that might change depending on the other PR if I understand correct.

niklasad1 · 2018-02-23T15:57:15Z

contract-client as a crate name is way too general and not a good description of its contents. Maybe registrar-wrapper or something which describes its intended use.

I agree and disagree 😄 as long as trait contains the ContractClient I want something in the name to indicate that. However if it's possible to remove ContractClient I'd be happy to rename it registrar-wrapper / registrar-client or something similar.

0x7CFE · 2018-02-23T15:57:32Z

@niklasad1, #7038 is almost complete and needs final review before merge. I hope I would not block you for a long time.

rphmeier · 2018-02-23T18:33:56Z

@niklasad1 or even rename the ContractClient trait to something else. It's a terrible name for the trait and the crate.

niklasad1 · 2018-02-26T12:47:59Z

@rphmeier
What do you think about re-naming the crate to registrar and re-naming the trait to RegistrarClient?

Because the crate should encapsulate the registrar with a uniform interface for client and it makes sense? Yepp, naming is one of the hardest problems in CS ⚡

rphmeier · 2018-02-26T16:27:43Z

@niklasad1 sounds fine to me

tomusdrw

Synchronous variant is not really used anywhere Is that expected?

Also in some places we could use more concrete types than Box<Future...>, but that's a minor thing, probably an improvement we can do in future.

niklasad1 · 2018-03-07T09:12:31Z

@tomusdrw

Synchronous variant is not really used anywhere Is that expected?

Yes, that is correct. The intention was to use Synchronous in èthcore/miner because it has its own interface to the registrar with different types. But I will look into it now when #7038 is merged.

Also in some places we could use more concrete types than Box<Future...>, but that's a minor thing, probably an improvement we can do in future.

Ok, do you want me create an issue for that?

tomusdrw · 2018-03-07T09:21:32Z

@niklasad1 don't bother, it's not a hot code anyway, just good that it will be possible in future.

andresilva

LGTM!

andresilva · 2018-03-12T20:05:23Z

registrar/src/registrar.rs

@@ -0,0 +1,61 @@
+use futures::{Future, future, IntoFuture};


Missing license header.

andresilva · 2018-03-12T20:11:25Z

registrar/src/registrar.rs

+
+use_contract!(registry, "Registry", "res/registrar.json");
+
+// Maps a domain name to IPv4 address


Maps a domain name to an Ethereum address

* Intoduced separate traits for asyncronous and syncronous contract clients * Removed registrar from ethcore::client * s/ContractClient/AsyncContractClient

* Use an associated type in the trait to determine syncronous or asyncronous communication * Export the types from contract-client crate * Document that "A" in the hash correspons to a DNS A Record A.K.A Address Record

5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Feb 22, 2018

5chdn added this to the 1.10 milestone Feb 22, 2018

rphmeier reviewed Feb 22, 2018

View reviewed changes

niklasad1 force-pushed the contract-client/refactor branch 2 times, most recently from 6477508 to caf05af Compare February 23, 2018 15:43

niklasad1 force-pushed the contract-client/refactor branch 2 times, most recently from 31eed4f to 80fe4a7 Compare February 23, 2018 16:38

niklasad1 force-pushed the contract-client/refactor branch 2 times, most recently from 9f62221 to fdd2068 Compare February 27, 2018 13:06

5chdn modified the milestones: 1.10, 1.11, 1.12 Mar 1, 2018

niklasad1 force-pushed the contract-client/refactor branch from fdd2068 to c0cb49a Compare March 5, 2018 11:32

tomusdrw approved these changes Mar 7, 2018

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 7, 2018

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Mar 7, 2018

niklasad1 force-pushed the contract-client/refactor branch from c0cb49a to ebfea3e Compare March 9, 2018 14:48

andresilva approved these changes Mar 12, 2018

View reviewed changes

niklasad1 added 9 commits March 12, 2018 21:14

Refactor usage of registry contract

343f98e

remove unsed file

e885a27

Rename methods & fix comments

7e7b3c5

refactor contract_client:

0925957

* Intoduced separate traits for asyncronous and syncronous contract clients * Removed registrar from ethcore::client * s/ContractClient/AsyncContractClient

make the tests compile and pass

f2007f3

omit changes in ethcore

cb55d6d

Use only one trait for ContractClient

69c27db

* Use an associated type in the trait to determine syncronous or asyncronous communication * Export the types from contract-client crate * Document that "A" in the hash correspons to a DNS A Record A.K.A Address Record

contract_client -> registrar

8aad8d7

address review feedback

cd9e303

niklasad1 force-pushed the contract-client/refactor branch from ebfea3e to cd9e303 Compare March 12, 2018 20:19

andresilva merged commit e0f71b0 into openethereum:master Mar 12, 2018

niklasad1 deleted the contract-client/refactor branch March 13, 2018 10:06

folsen mentioned this pull request May 21, 2018

remove registrar logic from the Client #7821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[contract-client] refactor #7978

[contract-client] refactor #7978

niklasad1 commented Feb 22, 2018 •

edited

Loading

parity-cla-bot commented Feb 22, 2018

rphmeier Feb 22, 2018

rphmeier Feb 22, 2018

rphmeier Feb 22, 2018

niklasad1 Feb 23, 2018

rphmeier commented Feb 22, 2018

rphmeier commented Feb 22, 2018 •

edited

Loading

niklasad1 commented Feb 23, 2018

niklasad1 commented Feb 23, 2018

0x7CFE commented Feb 23, 2018

rphmeier commented Feb 23, 2018

niklasad1 commented Feb 26, 2018 •

edited

Loading

rphmeier commented Feb 26, 2018

tomusdrw left a comment

niklasad1 commented Mar 7, 2018

tomusdrw commented Mar 7, 2018

andresilva left a comment

andresilva Mar 12, 2018

andresilva Mar 12, 2018


		use_contract!(registry, "Registry", "res/registrar.json");

		// Maps a domain name to IPv4 address

[contract-client] refactor #7978

[contract-client] refactor #7978

Conversation

niklasad1 commented Feb 22, 2018 • edited Loading

parity-cla-bot commented Feb 22, 2018

rphmeier Feb 22, 2018

Choose a reason for hiding this comment

rphmeier Feb 22, 2018

Choose a reason for hiding this comment

rphmeier Feb 22, 2018

Choose a reason for hiding this comment

niklasad1 Feb 23, 2018

Choose a reason for hiding this comment

rphmeier commented Feb 22, 2018

rphmeier commented Feb 22, 2018 • edited Loading

niklasad1 commented Feb 23, 2018

niklasad1 commented Feb 23, 2018

0x7CFE commented Feb 23, 2018

rphmeier commented Feb 23, 2018

niklasad1 commented Feb 26, 2018 • edited Loading

rphmeier commented Feb 26, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

niklasad1 commented Mar 7, 2018

tomusdrw commented Mar 7, 2018

andresilva left a comment

Choose a reason for hiding this comment

andresilva Mar 12, 2018

Choose a reason for hiding this comment

andresilva Mar 12, 2018

Choose a reason for hiding this comment

niklasad1 commented Feb 22, 2018 •

edited

Loading

rphmeier commented Feb 22, 2018 •

edited

Loading

niklasad1 commented Feb 26, 2018 •

edited

Loading