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

Light friendly dapps #5634

Merged
merged 5 commits into from
May 18, 2017
Merged

Light friendly dapps #5634

merged 5 commits into from
May 18, 2017

Conversation

rphmeier
Copy link
Contributor

Takes the first steps towards a futures-based dapps server and activates it for the light client.

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels May 16, 2017
@rphmeier rphmeier requested a review from tomusdrw May 16, 2017 10:46
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

LGTM, two grumbles.

@@ -60,7 +60,7 @@ fn should_return_503_when_syncing_but_should_make_the_calls() {

// then
response.assert_status("HTTP/1.1 503 Service Unavailable");
assert_eq!(registrar.calls.lock().len(), 4);
assert_eq!(registrar.calls.lock().len(), 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update the test name as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it wrong now? "the calls" is pretty ambiguous: it's definitely making some calls, although not as many as before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, makes sense.

.call_contract(BlockId::Latest, address, data)
fn call(&self, address: Address, data: Bytes) -> BoxFuture<Bytes, String> {
let res = (move || {
self.client.upgrade().ok_or_else(|| "Client not available".to_owned())?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd go with map instead of a closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the closure because of the ? operator usage here, it seemed easier than doing an and_then

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO would be cleaner with and_then:

futures::done(
    self.client.upgrade()
        .ok_or_else(|| "Client not available".to_owned())
        .and_then(|client| client.call_contract(BlockId::Latest, address, data))
).boxed()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 17, 2017
@gavofyork gavofyork merged commit b1eab69 into master May 18, 2017
@rphmeier rphmeier deleted the light-friendly-dapps branch May 18, 2017 12:56
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.

3 participants