New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove explicit send calls, part deux #4408

Merged
merged 22 commits into from Jun 6, 2018

Conversation

Projects
None yet
2 participants
@haarts
Contributor

haarts commented Jun 5, 2018

This is a continuation of #4376. The only thing changed is one commit fixing a bug in the original PR, I thought I could reopen the PR but I can't after I rebased the commits. Ping @SomberNight, @kyuupichan.

From the original:
The goal here was to make the code base easier to understand and more decoupled. All the ElectrumX protocol mentions are now in the network module. Except for two instances in the websockets module.

Functionally almost nothing changed. The only thing which changed is that in the commands module an optional timeout parameter could be passed. That parameter has been removed in favor of simpler code.

Contributed by FDF

haarts added some commits May 21, 2018

Rename synchronous_get to synchronous_send
This makes it more inline with the method 'send' of which
synchronous_send is the, well, synchronous version.
Move protocol strings from scripts to network
This is again a small step in the right direction. The network module is
going to accumulate more and more of these simple methods. Once
everything is moved into that module, that module is going to be split.

Note that I've left the scripts which use scripts/util.py alone. I
suspect the same functionality can be reached when using just
lib/network.py and that scripts/util.py is obsolete.
Remove protocol string from verifier and websocket
Websocket still has some references, that'll take more work to remove. Once the
network module has been split this should be easy.
I took the liberty to rename a variable to better show what it is.
Remove protocol strings from remainder
The naming scheme I'm following for the newly introduced methods in the network
module is: 'blockchain.<subject>.<action>' -> def <action>_(for|to)_<subject>
Move explicit protocol calls closer to each other
This makes it easier to keep track of the methods which are due to be
extracted.
Remove `send` when using `get_transaction`
This is the final step to formalize (the informal) interface of the network
module.
A chance of note is changed interface for async/sync calls. It is no longer
required to use the `synchronous_send` call. Merely NOT passing a callback
makes the call synchronous. I feel this makes the API more intuitive to work
with and easier to replace with a different network module.
Remove send from get_merkle_for_transaction
The pattern which emerged for calling the lambda yielded an slight refactor.
I'm not happy with the name for the `__invoke` method.
Remove optional timeout parameter
This parameter doesn't seem to be used a lot and removing it makes the
remaining calls easier. Potentionally a contentious choice!
Rename `broadcast` to `broadcast_transaction`
Doing so makes the method name consistent with the other ElectrumX protocol
method names.
Remove synchronous_send
Now every method is intuitive in what it does, no special handling required.
The `broadcast_transaction` method is weird. I've opted not to change the
return type b/c I found it hard to know what the exact consequences are. But
ideally this method should just works as all the other ElectrumX related
messages. On the other hand this shows nicely how you _can_ do something
differnt quite easy.
Rename the awkwardly name `__invoke` method
The new name reflects what it does.
Process the result of linter feedback
I've used flake8-diff (and ignored a couple of line length warnings).
Rename tx_response to on_tx_response
This fell through the cracks when this branch was rebased.
subscript_to_scripthash should be get_balance
An oversight while refactoring.
Add missing return statement
Without this statement the transaction would have been broadcasted twice.
@@ -1059,27 +1034,132 @@ def follow_chain(self, index):
def get_local_height(self):
return self.blockchain().height()
def synchronous_get(self, request, timeout=30):
def __wait_for(it):

This comment has been minimized.

@SomberNight

SomberNight Jun 5, 2018

Member

Even if it's not needed, could you make this a @staticmethod?

This comment has been minimized.

@haarts

haarts Jun 5, 2018

Contributor

This is indeed not needed but I'll gladly conform.

return result.get('result')
def __with_default_synchronous_callback(invocation, callback):

This comment has been minimized.

@SomberNight

SomberNight Jun 5, 2018

Member

same; staticmethod?

This comment has been minimized.

@haarts

haarts Jun 5, 2018

Contributor

idem

def get_transactions(self, transaction_hashes, callback=None):
command = 'blockchain.transaction.get'
messages = [(command, tx_hash) for tx_hash in transaction_hashes]

This comment has been minimized.

@SomberNight

SomberNight Jun 5, 2018

Member

(command, [tx_hash])

This comment has been minimized.

@haarts

haarts Jun 5, 2018

Contributor

good catch.

@SomberNight SomberNight merged commit e57e55a into spesmilo:master Jun 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 52.152%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment