Skip to content
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

[FIX] fix client-related behaviour #1479

Merged
merged 6 commits into from
Mar 20, 2020

Conversation

lampkin-diet
Copy link

Signed-off-by: Andrew Nikitin andrew.nikitin@dsr-corporation.com

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Mar 16, 2020

This pull request introduces 1 alert when merging 7dea3a9 into 95a13b4 - view on LGTM.com

new alerts:

  • 1 for Unused import

def send_ledger_status_to_client(self, lid, txn_s_n, v, p, merkle, protocol, client):
ls = LedgerStatus(lid, txn_s_n, v, p, merkle, protocol)
self.transmitToClient(ls, client)
def send_ledger_status_to_client(self, msg, client):
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be wrong, but I think that re-creation of a LedgerStatus was done explicitly in order to pass CURRENT_PROTOCOL_VERSION there. I think it wad important for some compatibility cases. Not sure if this is still a valid compatibility case, but I think it's worth double-checking.

Copy link
Author

Choose a reason for hiding this comment

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

The problem exactly in recreation process. If there was an error with LedgerStatus schema (as in test i send a message with merkleRoot parameter as null not str) then creation LedgerStatus object back will cause the same error too. I think, that in this case we need to reply to client the same message with error describing.

from stp_core.loop.eventually import eventually


@pytest.fixture(params=[zmq.REQ])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check other socket types? SUB, PUB, ROUTER, etc.

Copy link
Author

Choose a reason for hiding this comment

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

For other socket types we will have another behaviour. All of this cases will cause to ZMQErrors.

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Mar 17, 2020

This pull request introduces 1 alert when merging a772865 into 95a13b4 - view on LGTM.com

new alerts:

  • 1 for Unused import

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>
Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>
…ocol version

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>
@lgtm-com
Copy link

lgtm-com bot commented Mar 19, 2020

This pull request introduces 1 alert and fixes 1 when merging 7fb710a into 95a13b4 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for Unused import

@lampkin-diet lampkin-diet merged commit b4a8aa0 into hyperledger:master Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants