-
Notifications
You must be signed in to change notification settings - Fork 41
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
ink chain support #1785
Open
greg-schrammel
wants to merge
10
commits into
master
Choose a base branch
from
gregs/bx-1729-add-support-for-ink-l2
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
ink chain support #1785
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected] |
DanielSinclair
force-pushed
the
gregs/bx-1729-add-support-for-ink-l2
branch
6 times, most recently
from
December 18, 2024 07:53
aed3012
to
caecf31
Compare
fix: splice apechain curtis, ink sepolia testnet rpcs into default rpcs
add ink, inkSepolia chain migrations fix: apechain, apechainCurtis migrations in rainbowChains
DanielSinclair
force-pushed
the
gregs/bx-1729-add-support-for-ink-l2
branch
from
December 18, 2024 09:13
caecf31
to
67cbd3f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes BX-####
Figma link (if any):
What changed (plus any additional context for devs)
[email protected]
ink
andinkSepolia
hardcoded referencesInk ETH
; awaiting larger listinkSepolia
faucet URLuseSearchCurrencyLists
search supportNotable changes
avalancheFuji
andapechainCurtis
implementation issues and migrationsrainbowChains
, so included a newLOCAL_CHAINS
concept inreferences/chains
so that we can hardcode chains before backend rolls out chain support (mainly missing testnets). The testnet labels in Network settings required we added hardcoded references forchainsLabel
andchainsName
as well. This should be refactored in the future so we have one shared local chains reference, or expand backend results more easily.v1.5.53
release included a version bump for bothrainbowChains
anduserChains
persisted states, but lacked adequate migrations. This caused a loss of data for preferences for custom networks, network ordering and toggles, custom assets, RPCs, and more in late October.userChains
occured before this version as well, so we likely have had multiple data loss events.rainbowChains
I documented various state deltas and modified thev10
migration to properly merge theapechain
chain id. This will only be useful for users that still havev9
or below state; most users will have lost data here. I bumped to version 11 and addedv11
migration to mergeapechainCurtis
,ink
, andinkSepolia
. I added curtis in this 2nd migration because references to it were missing frominitialChains
that was used inv10
because it wasn't included as a backend network and the local chains concept didn't exist, so didn't wind up in state for users (despite other codebase references).userChains
issues were a bit more involved. TheuserChainsOrder
that was used for bootstrapping chain ordering state was inadvertently re-ordering networks, so new chains would appear in an ordering that didn't reflect backend's returned networks.userChains
was missingpersistOptions
entirely, so each time the version was bumped, we lost most of the data here. I created migrations forv1
throughv5
that naively reset state as was the previous behavior; it will be too challenging to roll the clock back on that to understand what the expected states should have been with each version bump.v5
migration foruserChains
follows the goal behavior ofrainbowChains
and tries to cleanly appendapechain
for users that haven't yet migrated.v6
does the same and appendsapechainCurtis
,ink
, andinkSepolia
. I successfully tested this flow from a local build ofv1.5.64
to these changes.Next steps
rainbowChains
anduserChains
as they are both error prone. We should replacerainbowChains
with an omnidirectional static import from thenetworks.json
file that is generated from backend. That way, we always know that the networks available to a build are bundled in that build, and can never diverge in state. We'll be able to adopt dynamic networks at the same time this way. We also userainbowChains
to add/remove/set the alternative RPC. This feature should be its own store; we should track the delta or RPC overrides without being destructive to the underlying backend data. This would allow us to change metadata from backend (like an RPC or native asset) without overriding user choice. If backend changes and user still relies on defaults (i.e. no reference to the network in preference overrides), then they inherit the new defaults; otherwise we take preference to their existing setting.userChains
has many of the same problems and really doesn't need to exist. We should capturecustomChains
in its own store, andchainOrdering
in its own store. There is no need to intermix mainnet chains with custom chains or user preferences. From the divergent, easier to maintain stores, we can create bootstrap a data model that can be relied upon in the codebase in the same way, without making tradeoffs for the underlying state integrity.Screen recordings / screenshots
What to test