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

Extension - Add/Remove Chains (Smoldot v.0.3.x) #428

Merged
merged 17 commits into from
Aug 3, 2021
Merged

Extension - Add/Remove Chains (Smoldot v.0.3.x) #428

merged 17 commits into from
Aug 3, 2021

Conversation

wirednkod
Copy link
Contributor

Fixes #427

@wirednkod wirednkod changed the title Extension - Add/Remove Chains Extension - Add/Remove Chains (Smoldot v.0.3.x) Jul 27, 2021
@wirednkod wirednkod requested a review from raoulmillais July 27, 2021 22:24
@wirednkod wirednkod marked this pull request as ready for review July 30, 2021 18:11
raoulmillais
raoulmillais suggested changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@raoulmillais raoulmillais left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In general I think this is a really good start 👍 The biggest issue I can see so far is that I really don't think we should be sending the spec message using the method field of the RPC message. We don't own the JSON RPC protocol - that is the domain of smoldot. We have our own type field in the MessageToManager which is designed to ask the background to do something - you should extend that instead and use provider.send() to send it from the ExtensionProvider.

Aside: Our logging strategy is going a bit all over the place in the extension, but I don't think you need to fix that in this PR: but we need a ticket for it though.

@wirednkod
Copy link
Contributor Author

Aside: Our logging strategy is going a bit all over the place in the extension, but I don't think you need to fix that in this PR: but we need a ticket for it though.

Add new issue #429

@wirednkod
Keep action without spec; Alter message to include 'spec' and use 'fo…
dc27cb1 
…rward' method for propagating it to background
@wirednkod wirednkod requested a review from raoulmillais August 2, 2021 20:08
raoulmillais
raoulmillais approved these changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@raoulmillais raoulmillais left a comment

Choose a reason for hiding this comment

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

Just one little stray comment to remove, and this is good to merge 👍

@wirednkod wirednkod merged commit 3493be4 into master Aug 3, 2021
@wirednkod wirednkod deleted the nik-chains-for-extension branch August 3, 2021 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raoulmillais raoulmillais raoulmillais 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.

Extension for smoldot v.0.3.X - Add/Remove Chains
2 participants
@wirednkod @raoulmillais