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

fix(apps-config)!: remove apps-config from sidecar #924

Merged
merged 3 commits into from
May 20, 2022

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented May 19, 2022

Breaking Change

Remove apps-config from sidecar. This means if you are using sidecar and you require custom substrate types, you must use the env variables mentioned here in order to inject them.

We will no longer be giving support for apps-config in sidecar for the following reasons:

  1. Too many third party packages, with no control over their maintenance, updates, or actions.

  2. Third party packages in apps-config has broken sidecars release one to many times (see v11.4.0), and instead of constantly making slight patches, changes or scripts to detect these issues that require not so clean methods, we are taking the more direct approach of just removing it completely.

  3. It can make maintenance very difficult, and overly complicated when polkadot-js packages dont align, exposing sidecar to bugs, and potential 'unknown' breaking changes.

  4. apps-config serves it's purpose for polkadot-js/apps, and should be kept to that. IMO, it has no place in sidecar.

One residual affect to this is we will no longer log if you are connected to a public node. We used a method from apps-config called createWsEndpoints which would give us all the public endpoints known to polkadot-js, then see if the address is the same as the one plugged in. We no longer will support this (but that being said it wasn't full proof either since its limited to the chains that polkadot-js knows about).

Dependency hell is a real thing, and it can be very tiring fixing, or identifying issues, instead of continuing to build out features. Hope you all understand :)

@TarikGul TarikGul requested a review from a team as a code owner May 19, 2022 20:53
Copy link
Contributor

@CurlyBracketEffect CurlyBracketEffect left a comment

Choose a reason for hiding this comment

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

Major quality of life improvment!

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

How does this impact parachains? I notice that many of the parachain specific type definition packages are now removed from the lock-file, but I'm a bit unsure about what this means for users wishing to connect to parachains.

@jsdw
Copy link
Collaborator

jsdw commented May 20, 2022

I didn't realise that removing apps-config would be so straightforward; nice!

How does this impact parachains? I notice that many of the parachain specific type definition packages are now removed from the lock-file, but I'm a bit unsure about what this means for users wishing to connect to parachains.

@TarikGul can confirm this, but afaik this just means that instead of being able to rely on type definitions being provided with sidecar via apps-config, users will have to provide and point to their own type definitions for a given parachain they're interested in connecting sidecar to (at least, to support operations on pre-v14 blocks).

This in turn means that we don't have to import third-party code via apps-config, which leads to needing resolutions (it's pretty likely that the apps-config packages will be lagging behind on any versions of things they use, so to avoid duplicating packages resolutions are used) and which can lead to breakage if those third parties make breaking changes that we don't spot. (I'm hoping I understood that right!)

@TarikGul
Copy link
Member Author

@dvdplm So @jsdw has it completely correct.

To also expand on the metadata, if a parachain team is leveraging V14 there shouldn't be any issues as the decoration can be done directly via the metadata, but anything pre-v14 that requires some customization will require teams to use any of the required fields as enviornment variables (as needed by the specific chain). Hence the breaking change.

SAS_SUBSTRATE_TYPES
SAS_SUBSTRATE_TYPES_CHAIN
SAS_SUBSTRATE_TYPES_BUNDLE
SAS_SUBSTRATE_TYPES_SPEC

@TarikGul
Copy link
Member Author

Two more things,

  1. in this PR I have removed the resolutions and Im not seeing any local issues or conflict of packages anymore :)

  2. I ran this against Moonbeam, and Acala as sanity checks and worked through the routes. It works exactly as should, and lets say there are certain rpc methods, or types that are not decorated sidecar will log all of those as warnings.

@TarikGul TarikGul merged commit e2e8b59 into master May 20, 2022
@TarikGul TarikGul deleted the tarik-remove-apps-config branch May 20, 2022 18:12
@dvdplm
Copy link
Contributor

dvdplm commented May 20, 2022

users will have to provide and point to their own type definitions for a given parachain they're interested in connecting sidecar to (at least, to support operations on pre-v14 blocks).

When we say "users"/"teams" here, we refer to end-users right? Everyone from solo-tinkerers to big exchange teams? Maybe this is a dumb question, but is it obvious from our docs where people should turn to find the necessary pre-V14 data they need?

I totally understand the desire to move away from depending on apps-config (which was never intended to be a public package and that we used in fault of a better alternative well knowing it was sub-optimal), but it's important that we don't push the hassle onto our users without providing clear guidance for them.

@TarikGul
Copy link
Member Author

users will have to provide and point to their own type definitions for a given parachain they're interested in connecting sidecar to (at least, to support operations on pre-v14 blocks).

When we say "users"/"teams" here, we refer to end-users right? Everyone from solo-tinkerers to big exchange teams? Maybe this is a dumb question, but is it obvious from our docs where people should turn to find the necessary pre-V14 data they need?

I totally understand the desire to move away from depending on apps-config (which was never intended to be a public package and that we used in fault of a better alternative well knowing it was sub-optimal), but it's important that we don't push the hassle onto our users without providing clear guidance for them.

Yea I would say it's always been pretty clear on how to inject the types into sidecar (Its documented in the chain integration guide, and the readme), but when it comes to how to get those types we don't give explicit instruction on where to find them. I also think we really shouldn't give explicit instructions either. My reasoning is not all external chains types are in apps-config, I've seen some chains just have a types repo at their org, others its deep in some SDK repo of theirs, and hard to find etc. My goal would be if a user is having an issue with their types via sidecar that they would file an issue specific to the chain they are trying to connect to then we can point them in the right direction.

This all being said, maybe it's worth me writing a message in the docs saying something like:
"If you are unsure where to get the types needed for a network, please refer to the chain builders of that network for direction, or file an issue in sidecar and we can point you in the right direction."

When we say "users"/"teams" here, we refer to end-users right? Everyone from solo-tinkerers to big exchange teams?

Yup exactly.

@TarikGul TarikGul mentioned this pull request May 24, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants