-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
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.
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); |
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 you update the test name as well?
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.
is it wrong now? "the calls" is pretty ambiguous: it's definitely making some calls, although not as many as before.
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.
Right, makes sense.
updater/src/updater.rs
Outdated
.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())? |
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 go with map
instead of a closure.
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 used the closure because of the ?
operator usage here, it seemed easier than doing an and_then
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 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()
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.
done
Takes the first steps towards a futures-based dapps server and activates it for the light client.