-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add networkStore and runtime support for networks #1835
base: master
Are you sure you want to change the base?
Conversation
…nd lots of clean up
…ctionality to hold all network data
@@ -60,17 +60,20 @@ export const estimateSwapGasLimit = async ({ | |||
const provider = getProvider({ chainId }); | |||
|
|||
if (!provider || !quote) { | |||
return getChainGasUnits(chainId).basic.swap; | |||
const chainGasUnits = networkStore.getState().getChainGasUnits(chainId); |
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.
can pull this line out to line 61 and delete the duplicate line on 72
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.
Can, but only wanted to grab it when/if we needed to in order to reduce calls to the network store.
balance: { amount: '', display: '' }, | ||
chainId: chainId, | ||
chainName: chainsLabel[chainId] as ChainName, | ||
chainName: chainLabel as ChainName, |
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.
this looks like an existing issue, but chainName
should be from getChainsName
instead of getChainsLabel
to be consistent
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.
also existing issue: on line 46 it looks like we are including native_asset_
in the uniqueId
which seems inconsistent with the way we handle uniqueId
s
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:
- token data: ✅
- swaps: ✅
- sends: ✅
- nfts: ✅
- custom networks: ✅
@@ -358,7 +358,7 @@ it('should be able to open token to buy input and select assets', async () => { | |||
expect(toBuyInputDaiSelected).toBeTruthy(); | |||
}); | |||
|
|||
it('should be able to type native amount on sell input', async () => { | |||
it.skip('should be able to type native amount on sell input', async () => { |
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.
Can you provide any info on this one? I can make a ticket but making sure this was intentional.
"anvil": "anvil --fork-url $(./scripts/get-rpc-url.sh mainnet) --fork-block-number 21574102 --block-base-fee-per-gas 5000000000 --block-gas-limit 30000000", | ||
"anvil": "anvil --fork-url $(./scripts/get-rpc-url.sh mainnet) --fork-block-number 21574102 --block-base-fee-per-gas 1000000000 --block-gas-limit 30000000", |
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.
Did this have a material effect on e2e?
console.log(chainId, existing); | ||
|
||
// add the rpc url to the chain if it exists | ||
if (existing) { | ||
const newUserPrferences = { | ||
...userPreferences, | ||
[chainId]: { | ||
...existing, | ||
activeRpcUrl: active ? rpcUrl : existing.activeRpcUrl, | ||
rpcs: { | ||
...existing.rpcs, | ||
[rpcUrl]: chain, | ||
}, | ||
}, | ||
}; | ||
|
||
console.log(newUserPrferences); |
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.
Can we add labels in front of the data in these logs if we're keeping them?
Fixes BX-1740, BX-1742, BX-1741, BX-1739, BX-1675
What changed (plus any additional context for devs)
backendNetworks
andcustomNetworks
customNetworks
now powers the pre-populated list of custom chains a user can addcreateQueryStore
architecturenetworkStore
that merges pre-existinguserChains
andrainbowChains
stores togethercreateSelector
andcreateParameterizedSelector
networkStore
store methodsreferences/chains
file with static build time accessorsScreen recordings / screenshots
// will try and capture a few screenshots tonight
What to test
https://www.notion.so/rainbowdotme/NetworkStore-Detailed-Test-Checklist-19eb001b85b48074a134d0ca093dbb9b