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

Provider injection #88

Closed
wants to merge 15 commits into from
Closed

Provider injection #88

wants to merge 15 commits into from

Conversation

axelchalon
Copy link
Contributor

@axelchalon axelchalon commented Jul 16, 2019

Close #58

Preparing the groundwork. Next up in this PR: integrating this in extension

@axelchalon
Copy link
Contributor Author

I imagine we also want to add a screen inside the extension where the user can choose the RPC endpoint, if it's different from ws://127.0.0.1:9944 ? Or is this non-urgent ?

@jacogr
Copy link
Member

jacogr commented Jul 17, 2019

Indeed, #15

We may want to take a peek at that first, at the very least it allows for 2 other quick hits - #10 & #85

(It is sensical to get that UI work done next, if nobody else grabs it in the meantime I will get around to it once apps is up and humming with 2.x again)

@axelchalon
Copy link
Contributor Author

axelchalon commented Jul 26, 2019

  • Typed sendMessage. TypeScript now checks that the message type is valid, that the payload corresponds to the message type's payload, and that the return type is the correct return type. e.g.
    const foo: boolean = await sendMessage('seed.create', { length: 12, type: 'ed25519' } }); // error, seed.create doesn't return a boolean
    const foo = await sendMessage('seed.create', true }); // error, seed.create expects { length, type }
  • Implement PostMessageProvider: PostMessageProvider uses the regular sendMessage function with method and params as request, and gets the response the usual way. Regarding subscription notifications (messages): PostMessageProvider receives them all and matches subscription id to handlers.
    Decided not to have PostMessageProvider encode in JSON and pass the JSON to the extension for multiple reasons, among them 1) would need yet another provider (extension would have to use WsRawProvider that forwards json strings to the provider) 2) if we want to implement ACL in the future, the extension would need to parse the incoming JSON of the request, 3) reusing the current loader architecture for requests/responses would mean having two ids being passed around, the usual sendMessage id & the jsonrpc id, which starts making things confusing

Still have to work out the handling of WsProvider connection/disconnection/reconnection and how it should affect the PostMessageProvider (right now it assumes the extension provider is always on) ; and have to actually test this PR against apps (next step in this PR), but overall I think the gist of it is here. so open for reviewing/comments!

@@ -0,0 +1,126 @@
// Copyright 2019 @polkadot/extension authors & contributors
Copy link
Member

Choose a reason for hiding this comment

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

I have just been scrolling through, so no in-depth comments yet - however, it would be good if we can actually move the provider (as to be injected) to extension-provider so it is actually easily re-usable by others as well.

(I can see that this file, as-is, can be shared - just the endpoint can be custom for a "quick start")

Copy link
Member

@jacogr jacogr Jul 26, 2019

Choose a reason for hiding this comment

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

And yes... need to do exactly the same with the injected signer. (In thinking about these 2, it may end up being in extension-inject then as sub-folders.) So extensions do the following -

  • include the package
  • use the top-level interface to inject into the page
  • create an instance of the signer for signing (or do their own)
  • create an instance of the provider (or ignore, or do their own)

One self-contained bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would be good if we can actually move the provider (as to be injected) to extension-provider so it is actually easily re-usable by others as well.

PostMessageProvider only sends messages to the window, it heavily relies on the logic (and message format) of page.js/content.js so that the messages are sent to the extension and received from the extension; as such I don't think it's very reusable by itself. Same for the injected signer, which basically only does sendRequest('extrinsic.sign', payload);. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest looking at how other extensions are using this - as it is, it is being used as-is. You do have a good suggestion buried in there though to make the message names slightly less generic for these case, i.e so it is explicit where it comes from and what it is for.

@axelchalon
Copy link
Contributor Author

Prior to the previous commit, the extension was using a single WsProvider, shared by all pages. Since we need active subscriptions made by the page to be closed when the page closes, and since there is no maintainable way of deriving the unsubscription method name from the subscription method name, I resorted to using one WsProvider per page. When the page closes, the associated underlying WsProvider disconnects.

Right now the WsProvider connects when the page makes an RPC request. Connecting or disconnecting the WsProvider from the Page (from the PostMessageProvider) isn't supported as it seems to me like a lot of overhead for little benefit.

@axelchalon axelchalon marked this pull request as ready for review August 23, 2019 14:48
@jacogr
Copy link
Member

jacogr commented Aug 29, 2019

Can we start pulling parts in? i.e. the typing changes - that we can pull in via another PR. (Actually unrelated and will clean this one . up some more). Would like to start getting this ready so it doesn't hang about.

@axelchalon
Copy link
Contributor Author

Sure sounds good, will do this now. I'll go about figuring out a strategy for reusability straight after

@jacogr
Copy link
Member

jacogr commented Aug 29, 2019

Perfect. Don't need to solve all the world's problems at once - happy to get improvements in as they come in. (Not a massive fan . of large PRs hanging about)

@jacogr
Copy link
Member

jacogr commented Feb 18, 2020

Closing, needed at some point, but not being worked on here or merge-able.

@jacogr jacogr closed this Feb 18, 2020
@jacogr jacogr deleted the ac-provider branch February 18, 2020 16:10
@amaury1093 amaury1093 mentioned this pull request Feb 24, 2020
@amaury1093 amaury1093 mentioned this pull request Feb 25, 2020
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Provider for extension injection
3 participants