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.

Sign up for GitHub

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

Jump to bottom

jsonrpc: Channel support #70

Merged
merged 14 commits into from
Jul 23, 2019
Merged

jsonrpc: Channel support #70

merged 14 commits into from
Jul 23, 2019

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Jul 22, 2019 •
edited
Loading

TODO:

  • Cleanup handler / websocket code
  • Some tests
  • Remove 'debug' / example code

@whyrusleeping
Copy link
Member

+1 looking good so far, will hold off on more detailed review until this is out of WIP

@magik6k
Copy link
Contributor Author

magik6k commented Jul 23, 2019

(tests pass for me locally)

This is ready for a review. For review order I'd recommend rpc_test.go -> websocket.go -> handler.go -> client.go

@magik6k magik6k marked this pull request as ready for review July 23, 2019 00:32
Kubuxu
Kubuxu approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

SGWM, it is complex and there are probably bugs we still have to catch.

whyrusleeping
whyrusleeping reviewed Jul 23, 2019
View reviewed changes
lib/jsonrpc/websocket.go
cancel()
}
}
if frame.ID != nil {
Copy link
Member

Choose a reason for hiding this comment

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

what if frame.id is nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't send back the response because this request is a notification and not a full call (https://www.jsonrpc.org/specification#notification)

whyrusleeping
whyrusleeping approved these changes Jul 23, 2019
View reviewed changes
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

the websockets code got a little more complicated, but still seems pretty reasonable

@magik6k magik6k merged commit 3ecc83e into master Jul 23, 2019
@Kubuxu Kubuxu deleted the feat/jsonrpc-chans branch July 24, 2019 07:00
magik6k added a commit that referenced this pull request Aug 10, 2020
@magik6k
Merge pull request #70 from filecoin-project/asr/refactor
937eba4 
extract GetRequiredPadding
nonsense pushed a commit that referenced this pull request Nov 6, 2020
@yusefnapora
Merge pull request #70 from filecoin-project/plan/drand-halting
459cd65 
plan: drand halting
dumikau pushed a commit to protofire/lotus that referenced this pull request Jun 8, 2023
@adlrocha
Rebase release/v1.20.0 (filecoin-project#70)
8d114e2 
This PR rebases eudico to use release/v1.20.0: filecoin-project#10184. This release should allow us to bundle the IPC actors within the `next` branch of `builtin-actors` and finally be able to load the IPC actors in Lotus.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kubuxu Kubuxu Kubuxu approved these changes

@whyrusleeping whyrusleeping whyrusleeping approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@magik6k @whyrusleeping @Kubuxu