-
Notifications
You must be signed in to change notification settings - Fork 161
feat: chainSpec based controller config; Types from apps-config #351
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
Conversation
dvdplm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to tinker with this a bit before I leave a proper review.
Co-authored-by: David <[email protected]>
|
Added a CHAIN_INTEGRATION.md guide that @joepetrowski may be interested in reviewing |
|
|
||
| ##### Basic balance transfer support | ||
|
|
||
| In order to support traditional balance transfers the chain's Sidecar endpoints should support account balance lookup, transaction submission, transaction material retrieval, and block queries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Even if we haven't decided the exact token standard, good to start somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be good to link to examples of how each of these look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dvdplm - what exactly do you have in mind for examples? My idea here was essentially to say "make sure to add the relevant controllers for these endpoints".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah maybe it's dumb but my thinking was to link to example implementations of the existing Polkadot/Kusama endpoints; a dev adding sidecar for their chain might want to peek at how you did those endpoints as theirs will likely be fairly similar?
Co-authored-by: joe petrowski <[email protected]>
|
|
||
| ##### Basic balance transfer support | ||
|
|
||
| In order to support traditional balance transfers the chain's Sidecar endpoints should support account balance lookup, transaction submission, transaction material retrieval, and block queries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be good to link to examples of how each of these look like.
| acc.push( | ||
| new controllers[controllerName](api, config.options.finalizes) | ||
| ); | ||
| acc.push(new controllers[controllerName](api, config.options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
maciejhirsz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor grumbles and comments
| boolean | ||
| ][]; | ||
|
|
||
| return controllersToInclude.reduce((acc, [controllerName, shouldMount]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One day JavaScript will get filterMap on arrays, one day... :)
Co-authored-by: David <[email protected]>
|
Responded to all comments. I think it is is looking pretty good. |
maciejhirsz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
dvdplm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny nit left!
| Skip to content | ||
| Search or jump to… | ||
|
|
||
| Pull requests | ||
| Issues | ||
| Marketplace | ||
| Explore | ||
|
|
||
| @emostov | ||
| paritytech | ||
| / | ||
| substrate - api - sidecar | ||
| 12 | ||
| 56 | ||
| 29 | ||
| Code | ||
| Issues | ||
| 20 | ||
| Pull requests | ||
| 1 | ||
| Actions | ||
| Security | ||
| 4 | ||
| Insights | ||
| Settings | ||
| substrate - api - sidecar / src / services / accounts / AccountsBalanceInfoService.ts / | ||
| @emostov | ||
| emostov Initial reply david review | ||
| Latest commit be66056 6 days ago | ||
| History | ||
| 2 contributors | ||
| @emostov @joepetrowski | ||
| We found potential security vulnerabilities in your dependencies. | ||
| You can see this message because you have been granted access to Dependabot alerts for this repository. | ||
|
|
||
| 95 lines(87 sloc) 2.38 KB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting glitch in the matrix here. :D
Co-authored-by: David <[email protected]>
Closes #334
Overview
This PR introduces the foundations of allowing Sidecar to work smoothly with FRAME based chains. It accomplishes this by
@polkadot/apps-configwhich are injected into the APIspecNameand hard coded config that aligns with thespecName.These changes will allow most FRAME chain builders to use Sidecar by just specifying end points their chain supports in the controllers config and keeping there chain types up to date in
@polkadot/apps-config.In this PR I also include controller configuration for Kulupu since it was requested in an issue (#350) and makes sense to have a live example to try out the setup.
Would be nice to get this through in the near future, since it will be a blocker for meaningful work in this repo.
Changes
Chain types
@polkadot/apps-configand use the inApiPromiseController config
ControllerConfiginsrc/types/chains-config)getControllersForSpecandgetControllersFromConfigfromsrc/chains-config/index.ts)src/controllers/index.tsnow exports a concretecontrollersobject so we can have better type safety when dynamically creating the controllers withgetControllersFromConfiginsrc/chains-config/index.tsfinalizesfiled that is used to decide if thegetFinalizedHeadshould be used in the blocks controllersubstrate-api-sidecar/src/controllers/blocks/BlocksController.ts
Line 94 in 0ac5573
Endpoints (services/ controllers)
Creation ofKulupuBlocksControllerwhich has new/kulupuBlocks/{head, :number}endpoint. I had to do this because there is no notion of finalized blocks since it is a simple PoW consensus (someone please correct if I am wrong). In the new controller only a couple lines here taken out to remove the?finalized=booleanquery param, but I created an entire other controller with a new endpoint name to keep things simple; However I am certainly open to other ideas, I just don't want the controllers to scale with a huge amount of conditionals as we add more chains.accounts/{accountId}/balance-infotokenSymbolfield in the response to reduce ambiguity on the token of the balances representedBlocksServiceis updated so it does not break when it encounters chains that do not have all the components to supportCalcFee.from_paramsOther changes
mainto declutter.Config=>SidecarConfigto reduce ambiguity against the new typeControllerConfigHow to try out the PR
Follow the instructions in the README: https://github.com/paritytech/substrate-api-sidecar#source-code-installation-and-usage and then start the service against Kulupu or Mandala by running
SAS_SUBSTRATE_WS_URL=wss://rpc.kulupu.corepaper.org/ws yarn start. You can then discover the available endpoints by visiting GEThttp://127.0.0.1:8080/Follow up work