-
-
Notifications
You must be signed in to change notification settings - Fork 256
SWAPS-2839 update bridge controllers for bitcoin #6454
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
SWAPS-2839 update bridge controllers for bitcoin #6454
Conversation
aganglada
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.
Sorry for the confusion on the spec. The ClientRequest: bit was only to outline it it's an onClientRequest
| import { toHex } from '@metamask/controller-utils'; | ||
| import { SolScope } from '@metamask/keyring-api'; | ||
| import type { |
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.
nice! I believe you can remove keyring-api from the package.json as well
| * @param chainId - The chain ID to check | ||
| * @returns True if the chain is a supported non-EVM chain, false otherwise | ||
| */ | ||
| export const isNonEvmChainId = ( |
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 wonder if there's an easier way, otherwise we need to keep updating this for every single new non EVM network
How about:
- Convert chainId to CAIP
- Check CAIP namespace for
eip155 - If
eip155, then it's EVM
What do you think?
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 like that idea! It does add some testing scope whereas the current functionality is already tested and working. Is this blocking? If not, I'd prefer to get these changes released to unblock the client PR, and then can update the function next week. Let me know what you think and I can add a // TODO comment @infiniteflower
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.
Yea this is non-blocking, fine with adding a TODO and addressing it in a follow up PR
| if (isSolanaChainId(chainId)) { | ||
| // Solana fees are stored in lamports (smallest units), need to convert to SOL | ||
| const decimals = 9; | ||
| feeInNative = calcTokenAmount(nonEvmFeesInNative ?? '0', decimals); |
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.
Why do we convert the SOL amount to lamports in bridge-controller.ts if we need to convert it back to SOL here?
|
|
||
| ### Removed | ||
|
|
||
| - Remove direct dependency on `@metamask/keyring-api` - no longer needed with unified Snap interface ([#6454](https://github.com/MetaMask/core/pull/6454)) |
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 dependency was not removed here, this is unaccurate
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.
Welp, I see it was actually listed twice. It's also in the changed section.
| - Update response handling to support new `transactionId` format from unified interface | ||
| - Support multiple response formats: string, `{ transactionId }`, `{ result: { signature } }`, and `{ signature }` | ||
| - Maintain backward compatibility with legacy response formats | ||
| - Rename transaction handling functions for clarity ([#6454](https://github.com/MetaMask/core/pull/6454)) |
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.
These changelogs are meant for users of the package, not internal contributors. Please don't document internal-only changes such as internal function renames. It makes it much harder to tell what has actually changed for anyone using this package.
| ### Changed | ||
|
|
||
| - Update transaction submission to use new unified Snap interface for all non-EVM chains ([#6454](https://github.com/MetaMask/core/pull/6454)) | ||
| - Replace `signAndSendTransactionWithoutConfirmation` with `ClientRequest:signAndSendTransaction` method |
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 is a breaking change.
I'm told in practice it probably doesn't require client changes because our snaps support ClientRequest:signAndSendTransaction already. But we should still be documenting it properly here so that it's clearly understood that there is a compatibility change here.
| - Update chain ID handling for non-EVM chains ([#6454](https://github.com/MetaMask/core/pull/6454)) | ||
| - Add fallback chain ID (`0x0`) when CAIP format can't be converted to hex for source chains | ||
| - Add fallback chain ID (`0x1`) for non-EVM destination chains | ||
| - Update `getClientRequest` to create proper requests for all non-EVM chains ([#6454](https://github.com/MetaMask/core/pull/6454)) |
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 seems like an internal change as well (a subset of "Add support for Bitcoin bridge transactions"). No need to document internal changes like this.
|
|
||
| - **BREAKING:** Rename fee handling for non-EVM chains ([#6454](https://github.com/MetaMask/core/pull/6454)) | ||
| - Replace `SolanaFees` type with `NonEvmFees` type | ||
| - Replace `solanaFeesInLamports` field with `nonEvmFeesInNative` field |
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.
It's unclear what is being renamed here, is this a property on a return value? The context made it seem like it was a package export or a method or something. More context would be appreciated here
| - **BREAKING:** Rename fee handling for non-EVM chains ([#6454](https://github.com/MetaMask/core/pull/6454)) | ||
| - Replace `SolanaFees` type with `NonEvmFees` type | ||
| - Replace `solanaFeesInLamports` field with `nonEvmFeesInNative` field | ||
| - Update `#appendSolanaFees` to `#appendNonEvmFees` to support all non-EVM chains |
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 line seems like an internal-only change
| - Replace `getFeeForTransaction` with `computeFee` method that returns fees in native token units | ||
| - Update fee calculation to handle different unit conversions per chain | ||
| - Support fee computation for Bitcoin and Solana chains | ||
| - Update quote validation to support Bitcoin-specific trade data format ([#6454](https://github.com/MetaMask/core/pull/6454)) |
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.
Nit: This is fine, but maybe better to move these two entries ("Update quote validation" and "Update selectors and utilities") as sub-bullets of "Add support for Bitcoin bridge transactions". It seems like a detail of that addition, I don't think it's as helpful to frame it as a standalone change.
| - Update `#appendSolanaFees` to `#appendNonEvmFees` to support all non-EVM chains | ||
| - The `nonEvmFeesInNative` field stores fees in the smallest units for each chain (lamports for Solana, satoshis for Bitcoin) | ||
| - Update Snap methods to use new unified interface for non-EVM chains ([#6454](https://github.com/MetaMask/core/pull/6454)) | ||
| - Replace `getFeeForTransaction` with `computeFee` method that returns fees in native token units |
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 seems like a breaking change as well. This is a change in the expectations we have of snaps this controller inter-operates with.
| snapId: snapId as never, | ||
| origin: 'metamask', | ||
| handler: 'onRpcRequest' as never, | ||
| handler: 'onClientRequest' as never, |
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.
Why cast to never 🤔 As the name might imply, never should never be resolved on any of our types. It's meant to alert when a conditional type is broken.
) ## Explanation This reverts commit 1aae93d, which was accidentally released as part of v44 with a bunch of undocumented breaking changes. ## References See #6454 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
Explanation
This PR extends the bridge controller to support Bitcoin transactions, building on the existing Solana support to create a more generic non-EVM chain handling system.
Current state: The bridge controller currently only supports EVM chains and Solana for cross-chain transactions.
Solution: This PR:
instead of SolanaFees, handleNonEvmTx instead of handleSolanaTx)
that work across all non-EVM chains
Key changes:
References
Checklist