- 
                Notifications
    
You must be signed in to change notification settings  - Fork 0
 
Proposed changes for DataHighway integration with Polkadot.js Apps #2
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
Proposed changes for DataHighway integration with Polkadot.js Apps #2
Conversation
| isDisabled: true, | ||
| text: t('rpc.test.datahighway.spreehafen', 'Spreehafen', { ns: 'apps-config' }), | ||
| providers: { | ||
| MXC: 'wss://spreehafen.datahighway.com' | 
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.
we need to find out the relevance of this being MXC or DataHighway
| info: 'datahighway-harbour', | ||
| text: t('rpc.test.datahighway-harbour', 'DataHighway Harbour', { ns: 'apps-config' }), | ||
| info: 'datahighway-harbour-chain-testnet', | ||
| text: t('rpc.test.datahighway.harbour', 'DataHighway Harbour', { ns: 'apps-config' }), | 
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.
from a maintenance perspective, it might be easier if this was named datahighway-harbour instead of datahighway.harbour since find/replace would be quicker. what do you think?
(same applies to datahighway.spreehafen and datahighway.westlake)
| ['DataHighwayChain', nodeWestlake], | ||
| ['DataHighway Node', nodeDataHighway], | ||
| ['DataHighway Parachain Collator', nodeDataHighway], | ||
| ['DataHighway', nodeDataHighwayDefault], // TODO - deprecated? | 
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.
we should remove all lines that i've labelled // TODO - deprecated? if they're not used anymore
| minmax: [0, undefined], | ||
| types: { | ||
| Date: 'i64', | ||
| AccountInfo: "AccountInfoWithDualRefCount", | 
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 tried adding and removing this. App works without it due to recent changes. Let me reverify this.
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.
@ltfschoen I am merging this. I will test and make corrections in my branch.
          
 the CI tests are failing for some reason, so we need to resolve that first  | 
    
          
 i just pushed a commit to see if that fixes it, will wait for CI to test again  | 
    
          
 sure  | 
    
          
 i'm not familiar with why we're getting "Semgrep / check (pull_request) Failing", perhaps that's setup for PRs into polkadot-js/apps instead of our fork of it, so it may not matter if it doesn't pass until we change the PR target to polkadot-js/apps instead  | 
    
Requires DataHighway-DHX/node#201