-
Notifications
You must be signed in to change notification settings - Fork 360
Add get chain info listener #3062
Conversation
…e for the web3 equivalent
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Pull Request Test Coverage Report for Build 1504215715
💛 - Coveralls |
Deployment links
|
|
E2E Tests Passed ✅ |
katspaugh
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.
Muy bien!
| communicator?.on(Methods.getChainInfo, async () => { | ||
| const { nativeCoin } = getNetworkInfo() | ||
| const chainId = parseInt(networkId, 10) | ||
| const network = getNetworkName() |
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.
@iamacook in your chains PR, do these functions still exist?
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.
The current issues regarding using Redux likely affect every change I have made so far, so I can take the opportunity to start again with these changes included.
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 are aware of that and we are planning to change this in @iamacook PR. Once this one is unblocked we will push this changed to his PR
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 think @iamacook has a selector or something that we can use
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.
If this gets merged prior, it would be a lot easier for me to handle. Either way, I'll be happy to help fix any merge conflicts.
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.
Yes, we are going to merge this tomorrow or on monday at the latest I think
|
@katspaugh @iamacook Thank you for your review. Next step for us would be sync with Aaron, as we already checked his PR about using the |
|
@dasanra, I have been unable to sort the issues I showed you last week regarding the store so I'll be changing my approach in a new branch. Feel free to merge this. I'll try to keep the logic as close to how it currently is instead |
What it solves
Resolves safe-global/safe-react-apps#185
How this PR fixes it
We are going to get the chain info from the apps using the SDK (safe-global/safe-apps-sdk#258). So in this PR we are adding the listener for retrieving the required info
How to test it