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

Separate chainAdapterReady from chainModuleReady. #26

Merged
merged 2 commits into from
Apr 22, 2020

Conversation

jnaviask
Copy link
Collaborator

Description

We use a global Subject called "app.chainReady" in dynamic components to wait until the entire chain has initialized before loading additional dynamic data.

This Subject was also being used in the SubstrateAccounts module to handle deferred initialization of the "ChainInfo" module, i.e. the SubstrateChain class. However, the app.chainReady waits for the entire chain adapter to load, not just the app.chain.chain module.

To fix this, I separated it into two different Subjects: app.chainAdapterReady (same as original app.chainReady), and app.chainModuleReady (used in Accounts and possibly other future modules to defer initialization).

Motivation and Context

This bug was causing loading errors on production: the app.chainReady wouldn't load until the SubstrateCollective module loaded, but the SubstrateCollective module assumes that account deferred initializations have already resolved (i.e. that the app.chainModuleReady has resolved), so it finds itself unable to query for balances and throws an error.

How Has This Been Tested?

Reproduced bug in dev environment, verified fix.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • My change should be included in the release notes.
  • I have updated the documentation accordingly.
  • I have linted the code locally prior to submission.

Copy link
Contributor

@drewstone drewstone left a comment

Choose a reason for hiding this comment

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

Looks good, we can def cut down on some redundancy here

client/scripts/controllers/chain/community/main.ts Outdated Show resolved Hide resolved
client/scripts/controllers/chain/edgeware/main.ts Outdated Show resolved Hide resolved
client/scripts/controllers/chain/cosmos/main.ts Outdated Show resolved Hide resolved
client/scripts/controllers/chain/ethereum/main.ts Outdated Show resolved Hide resolved
client/scripts/controllers/chain/near/main.ts Outdated Show resolved Hide resolved
client/scripts/controllers/chain/substrate/main.ts Outdated Show resolved Hide resolved
@drewstone drewstone merged commit cc80aaa into master Apr 22, 2020
@raykyri raykyri deleted the jake.chain-module-ready branch October 14, 2020 19:08
jnaviask pushed a commit that referenced this pull request Jul 15, 2022
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.

2 participants