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

ink chain support #1785

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

greg-schrammel
Copy link
Contributor

@greg-schrammel greg-schrammel commented Dec 12, 2024

Fixes BX-####
Figma link (if any):

What changed (plus any additional context for devs)

  • Upgraded to [email protected]
  • Added ink and inkSepolia hardcoded references
  • Added chain icon in sister PR: feat: ink, ink sepolia assets#105
  • Created a favorites store migration to include Ink ETH; awaiting larger list
  • Added inkSepolia faucet URL
  • Added initial useSearchCurrencyLists search support
  • Added default slippage and bips; to be followed up in seperate swaps PR

Notable changes

  • Discovered and corrected avalancheFuji and apechainCurtis implementation issues and migrations
  • Discovered that chains not returned from backend no longer are referenced by rainbowChains, so included a new LOCAL_CHAINS concept in references/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 for chainsLabel and chainsName as well. This should be refactored in the future so we have one shared local chains reference, or expand backend results more easily.
  • Discovered that the v1.5.53 release included a version bump for both rainbowChains and userChains 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.
    • Previous version bumps of userChains occured before this version as well, so we likely have had multiple data loss events.
    • For rainbowChains I documented various state deltas and modified the v10 migration to properly merge the apechain chain id. This will only be useful for users that still have v9 or below state; most users will have lost data here. I bumped to version 11 and added v11 migration to merge apechainCurtis, ink, and inkSepolia. I added curtis in this 2nd migration because references to it were missing from initialChains that was used in v10 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).
    • The userChains issues were a bit more involved. The userChainsOrder 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 missing persistOptions entirely, so each time the version was bumped, we lost most of the data here. I created migrations for v1 through v5 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.
    • The v5 migration for userChains follows the goal behavior of rainbowChains and tries to cleanly append apechain for users that haven't yet migrated.
    • v6 does the same and appends apechainCurtis, ink, and inkSepolia. I successfully tested this flow from a local build of v1.5.64 to these changes.

Next steps

  • Remember that when you test migrations, you'll need to fully remove the extension, downgrade and bootstrap a new wallet setup, then refresh the extension with build folder changes. Otherwise you'll end up in migration escape and won't encounter the same flow as users.
  • To make the above more sane, I propose we create a snapshot unit test for the important Zustand stores that allows us to codify expected state, and test various migrations to the intended goal state.
  • Generally, it seems like we need to blow away the idea of rainbowChains and userChains as they are both error prone. We should replace rainbowChains with an omnidirectional static import from the networks.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 use rainbowChains 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 capture customChains in its own store, and chainOrdering 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

Copy link

linear bot commented Dec 12, 2024

Copy link

socket-security bot commented Dec 12, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/[email protected] None +2 5.53 MB awkweb, jmoxey
npm/[email protected] None 0 195 kB jmoxey

🚮 Removed packages: npm/[email protected]

View full report↗︎

@DanielSinclair DanielSinclair force-pushed the gregs/bx-1729-add-support-for-ink-l2 branch 6 times, most recently from aed3012 to caecf31 Compare December 18, 2024 07:53
@DanielSinclair DanielSinclair force-pushed the gregs/bx-1729-add-support-for-ink-l2 branch from caecf31 to 67cbd3f Compare December 18, 2024 09:13
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