Feature/51/subscription to minimal consensus #55
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- tests fail, but no panics
- epochs are mocked, but subscription mock does not work
- right now method name is parsed ok, but array is not
- tests does not pass but orchestratorServer have minimal implementation
- for some reason notifier fails
- for some reason notifier fails
- for some reason notifier fails.. one of the msg during the handshake of subscription is empty and it produces err
- some kind of fallback on unmarshaling the result which is empty, TODO: debug if this really happen in prod env
- still backend gives me error when trying to subscribe
atif-konasl
reviewed
Apr 19, 2021
- for some reason using publicFilterApi solves the subscription problem
- channel does not receive minimal consensus when listening to subscription
- subscription channel does not work yet
- removed insecure api
- orchestrator mocked in stress ethash is not passed everywhere
- very weird behaviour
- moved to simplier struct, but test still fails on epoch 2
- test pass
- execution engine should work with orchestrator
atif-konasl
reviewed
Apr 21, 2021
There was a problem hiding this comment.
Overall Review:
RP looks good but few vital concerns are:
- Subscriber should have retry option.
- Subscriber should not be maintained in
PandoraSealer. Because, remote sealer only does sealing task. - When pandora restarts every time, subscriber always starting from epoch 0 which is not ideal solution. Epoch should be latest according to its latest canonical block saved blockchain.
- For testing our implementation, we should not add/remove code in exiting implementation.
- We should maintain new package for our implementation, because we are implementing on top of active
gethrepo.
Travis CI Not Passed
|
about concerns:
|
- fix for ipc endpoint
- tests do not have import cycle
- stress test runs
- go lint pass
Tried to remove gobin modifications
blazejkrzak
changed the title
This was referenced Apr 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes will allow estabilish Pandora subscription to Orchestrator