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

fix: Bridge{,Status}Controller state types and restricted-path imports #28971

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Dec 5, 2024

Motivation

ControllerState types should represent the entire state at root level that is passed to BaseController and RestrictedMessengerController.

The current set of types for Bridge{,Status}Controller state have issues:

  • There is no dedicated type that represents the controller state.
    • This is inconvenient for typing e.g. selectors.
    • This presents an unexpected API for downstream consumers which could lead to silent failures.
  • The messenger types for the getState action, stateChange` event, and other messages that reference the controller's state are incorrect.

For more information on writing controllers, see:

https://github.com/MetaMask/core/blob/main/docs/writing-controllers.md

Description

Renames:

  • BridgeControllerState type to BridgeState
  • BridgeStatusControllerState type to BridgeStatusState.
  • DEFAULT_BRIDGE_CONTROLLER_STATE constant to DEFAULT_BRIDGE_STATE.
  • DEFAULT_BRIDGE_STATUS_CONTROLLER_STATE constant to DEFAULT_BRIDGE_STATUS_STATE.
  • BridgeState Redux slice type to BridgeSlice.

Defines new BridgeControllerState, BridgeStatusControllerState types and DEFAULT_BRIDGE_CONTROLLER_STATE, DEFAULT_BRIDGE_STATUS_CONTROLLER_STATE constants that represent the corresponding controllers' state at root level.

Moves Bridge{,Status}Controller types and constants from app/scripts/{controllers,shared}, ui/pages/bridge to shared/types/bridge, resolving restricted-path imports and circular dependencies.

Note

See PR comments for a walkthrough of the changes.

Open in GitHub Codespaces

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

github-actions bot commented Dec 5, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@MajorLift MajorLift changed the title Fix broken BridgeControllerState, BridgeStatusControllerState types fix: BridgeControllerState, BridgeStatusControllerState types do not represent full state object Dec 5, 2024
@MajorLift MajorLift added the team-tiger Tiger team (for tech debt reduction + performance improvements) label Dec 5, 2024
@MajorLift MajorLift self-assigned this Dec 5, 2024
@MajorLift MajorLift force-pushed the jongsun/fix/bridge-controller/241205-fix-BridgeControllerState-type branch from 601c055 to ff835da Compare December 5, 2024 19:21
@MajorLift MajorLift requested a review from a team December 5, 2024 19:22
@MajorLift MajorLift marked this pull request as ready for review December 5, 2024 19:52
@metamaskbot
Copy link
Collaborator

Builds ready [3a3754b]
Page Load Metrics (1940 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17342221194512661
domContentLoaded16622185191112962
load17302206194012761
domInteractive266537126
backgroundConnect766282010
firstReactRender15111282412
getState793551566732
initialActions00000
loadScripts12621719146511857
setupStore76112126
uiStartup196229802298240115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -20 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 75 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [f9e9539]
Page Load Metrics (1874 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint54221541819318153
domContentLoaded16322139183711756
load16602156187412058
domInteractive2610437178
backgroundConnect1187422110
firstReactRender1798342814
getState823131658139
initialActions01000
loadScripts12391741144311254
setupStore68012168
uiStartup194626692285242116
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -20 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 75 Bytes (0.00%)

@MajorLift MajorLift force-pushed the jongsun/fix/bridge-controller/241205-fix-BridgeControllerState-type branch from f9e9539 to 768412e Compare December 11, 2024 09:24
@MajorLift MajorLift added type-tech-debt Technical debt and removed team-wallet-framework labels Dec 11, 2024
@MajorLift MajorLift changed the title fix: BridgeControllerState, BridgeStatusControllerState types do not represent full state object fix: Bridge{,Status}Controller state types, circular dependencies Dec 11, 2024
@MajorLift MajorLift changed the title fix: Bridge{,Status}Controller state types, circular dependencies fix: Bridge{,Status}Controller state types, restricted-path dependencies Dec 11, 2024
@MajorLift MajorLift added the team-extension-platform Extension Platform team label Dec 11, 2024
@MajorLift MajorLift marked this pull request as draft December 11, 2024 14:12
@MajorLift MajorLift force-pushed the jongsun/fix/bridge-controller/241205-fix-BridgeControllerState-type branch 5 times, most recently from 9cc4bc8 to 2d5f4e2 Compare December 11, 2024 15:09
@MajorLift MajorLift changed the title fix: Bridge{,Status}Controller state types, restricted-path dependencies fix: Bridge{,Status}Controller state types and circular dependencies Dec 11, 2024
@MajorLift MajorLift force-pushed the jongsun/fix/bridge-controller/241205-fix-BridgeControllerState-type branch from 2d5f4e2 to 0126ed3 Compare December 11, 2024 15:16
@MajorLift MajorLift marked this pull request as ready for review December 11, 2024 15:22
@MajorLift MajorLift force-pushed the jongsun/fix/bridge-controller/241205-fix-BridgeControllerState-type branch from 0126ed3 to 4e4ae1d Compare December 11, 2024 15:29
@MajorLift MajorLift force-pushed the jongsun/fix/bridge-controller/241205-fix-BridgeControllerState-type branch from 2aa81b0 to 7a2578e Compare December 11, 2024 15:49
Copy link
Contributor Author

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Walkthrough for reviewers:

Comment on lines 24 to 41
export type BridgeControllerState = {
bridgeState: BridgeState;
};

export type BridgeState = {
bridgeFeatureFlags: BridgeFeatureFlags;
srcTokens: Record<string, SwapsTokenObject>;
srcTopAssets: { address: string }[];
destTokens: Record<string, SwapsTokenObject>;
destTopAssets: { address: string }[];
quoteRequest: Partial<QuoteRequest>;
quotes: (QuoteResponse & L1GasFees)[];
quotesInitialLoadTime?: number;
quotesLastFetched?: number;
quotesLoadingStatus?: RequestStatus;
quoteFetchError?: string;
quotesRefreshCount: number;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Rename BridgeControllerState to BridgeState
  • Define new BridgeControllerState

Comment on lines +163 to +179
export type BridgeStatusState = {
txHistory: Record<SourceChainTxMetaId, BridgeHistoryItem>;
};

export type BridgeStatusControllerState = {
bridgeStatusState: BridgeStatusState;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Rename BridgeStatusControllerState to BridgeStatusState
  • Define new BridgeStatusControllerState

Comment on lines +13 to 14
export const DEFAULT_BRIDGE_STATE: BridgeState = {
bridgeFeatureFlags: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Rename DEFAULT_BRIDGE_CONTROLLER_STATE to DEFAULT_BRIDGE_STATE

Comment on lines +39 to +40
export const DEFAULT_BRIDGE_CONTROLLER_STATE: BridgeControllerState = {
bridgeState: { ...DEFAULT_BRIDGE_STATE },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Define new DEFAULT_BRIDGE_CONTROLLER_STATE

Comment on lines +10 to 17
export const DEFAULT_BRIDGE_STATUS_STATE: BridgeStatusState = {
txHistory: {},
};

export const DEFAULT_BRIDGE_STATUS_CONTROLLER_STATE: BridgeStatusControllerState =
{
txHistory: {},
bridgeStatusState: DEFAULT_BRIDGE_STATUS_STATE,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Rename DEFAULT_BRIDGE_STATUS_CONTROLLER_STATE to DEFAULT_BRIDGE_STATUS_STATE
  • Define new DEFAULT_BRIDGE_STATUS_CONTROLLER_STATE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contents of this file were moved to shared/types/bridge.ts

Comment on lines 55 to 195
symbol: string;
name: string;
decimals: number;
icon?: string;
};

export type QuoteRequest = {
walletAddress: string;
destWalletAddress?: string;
srcChainId: ChainId;
destChainId: ChainId;
srcTokenAddress: string;
destTokenAddress: string;
srcTokenAmount: string; // This is the amount sent
slippage: number;
aggIds?: string[];
bridgeIds?: string[];
insufficientBal?: boolean;
resetApproval?: boolean;
refuel?: boolean;
};

type Protocol = {
name: string;
displayName?: string;
icon?: string;
};

enum ActionTypes {
BRIDGE = 'bridge',
SWAP = 'swap',
REFUEL = 'refuel',
}

type Step = {
action: ActionTypes;
srcChainId: ChainId;
destChainId?: ChainId;
srcAsset: BridgeAsset;
destAsset: BridgeAsset;
srcAmount: string;
destAmount: string;
protocol: Protocol;
};

type RefuelData = Step;

export type Quote = {
requestId: string;
srcChainId: ChainId;
srcAsset: BridgeAsset;
// This is amount sent - metabridge fee, however, some tokens have a fee of 0
// So sometimes it's equal to amount sent
srcTokenAmount: string;
destChainId: ChainId;
destAsset: BridgeAsset;
destTokenAmount: string;
feeData: Record<FeeType.METABRIDGE, FeeData> &
Partial<Record<FeeType, FeeData>>;
bridgeId: string;
bridges: string[];
steps: Step[];
refuel?: RefuelData;
};

export type QuoteResponse = {
quote: Quote;
approval: TxData | null;
trade: TxData;
estimatedProcessingTimeInSeconds: number;
};

export enum ChainId {
ETH = 1,
OPTIMISM = 10,
BSC = 56,
POLYGON = 137,
ZKSYNC = 324,
BASE = 8453,
ARBITRUM = 42161,
AVALANCHE = 43114,
LINEA = 59144,
}

export enum FeeType {
METABRIDGE = 'metabridge',
REFUEL = 'refuel',
}
export type FeeData = {
amount: string;
asset: BridgeAsset;
};
export type TxData = {
chainId: ChainId;
to: string;
from: string;
value: string;
data: string;
gasLimit: number | null;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the former contents of ui/pages/bridge/types.ts.

They were moved to resolve restricted-path import and circular dependency issues.

Comment on lines 29 to -32
import {
BridgeControllerState,
BridgeFeatureFlagsKey,
L1GasFees,
QuoteRequest,
QuoteResponse,
TxData,
// TODO: Remove restricted import
// eslint-disable-next-line import/no-restricted-paths
} from '../../../../ui/pages/bridge/types';
Copy link
Contributor Author

@MajorLift MajorLift Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

14 violations of import/no-restricted-paths are resolved in this PR.

@MajorLift MajorLift requested a review from a team as a code owner December 11, 2024 22:59
@MajorLift MajorLift added team-bridge and removed team-extension-platform Extension Platform team labels Dec 11, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [c8b4228]
Page Load Metrics (1748 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15662054174712259
domContentLoaded15572041171111857
load15662055174813263
domInteractive248247199
backgroundConnect12173403617
firstReactRender16106553215
getState585272713
initialActions01000
loadScripts1125148912929646
setupStore681152110
uiStartup180927952122271130
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 31 Bytes (0.00%)
  • ui: -251 Bytes (-0.00%)
  • common: -504 Bytes (-0.01%)

@MajorLift MajorLift enabled auto-merge December 11, 2024 23:41
@dbrans
Copy link
Contributor

dbrans commented Dec 12, 2024

We’re removing the Extension Platform team from the reviewers list for now. If you need our input, feel free to re-add us—we’re happy to help!

@dbrans dbrans removed the request for review from a team December 12, 2024 17:13
@MajorLift MajorLift force-pushed the jongsun/fix/bridge-controller/241205-fix-BridgeControllerState-type branch from c8b4228 to 80ba0d3 Compare December 12, 2024 17:19
@metamaskbot
Copy link
Collaborator

Builds ready [80ba0d3]
Page Load Metrics (1621 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26319161486425204
domContentLoaded14001899160213163
load14081915162113264
domInteractive24140453014
backgroundConnect65717126
firstReactRender1577352411
getState44821178
initialActions01000
loadScripts10451420119510751
setupStore65311115
uiStartup16402203185215575
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 31 Bytes (0.00%)
  • ui: -251 Bytes (-0.00%)
  • common: -504 Bytes (-0.01%)

@MajorLift MajorLift force-pushed the jongsun/fix/bridge-controller/241205-fix-BridgeControllerState-type branch from 80ba0d3 to 88c5311 Compare December 13, 2024 18:51
@metamaskbot
Copy link
Collaborator

Builds ready [d993ba2]
Page Load Metrics (1749 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33419961681331159
domContentLoaded15541976172111957
load16142001174911656
domInteractive257340167
backgroundConnect795362311
firstReactRender16106622813
getState4271053
initialActions01000
loadScripts11781542130110149
setupStore683172211
uiStartup179927932086249119
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 31 Bytes (0.00%)
  • ui: -251 Bytes (-0.00%)
  • common: -504 Bytes (-0.01%)

@MajorLift
Copy link
Contributor Author

Superseded by #29254

@MajorLift MajorLift closed this Jan 6, 2025
auto-merge was automatically disabled January 6, 2025 16:08

Pull request was closed

@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-bridge team-swaps team-tiger Tiger team (for tech debt reduction + performance improvements) type-tech-debt Technical debt
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants