(Deposit/Withdraw) Support External Url Providers #3310
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
JoseRFelix
force-pushed
the
jose/fe-471-support-external-url-providers
branch
from
bc68e7d
to
8f08f21
Compare
JoseRFelix
force-pushed
the
jose/fe-471-support-external-url-providers
branch
from
e774bf8
to
19430fb
Compare
JoseRFelix
changed the title
@jonator Added an IBC external url function for TFM on my latest commit |
I considered it, but I'm hesitant to include it to avoid having to manage the bundle size |
…external-url-providers
WalkthroughThe updates to the bridge package involve significant enhancements to the Axelar and IBC modules, including the introduction of new functions for generating external URLs, improved mock data handling, and schema updates for chain types. The changes also standardize chain ID representations and improve error messages for unsupported chains. Additionally, new tests have been added to ensure the robustness of these functionalities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AxelarBridgeProvider
participant MockAxelarAssets
participant MockAxelarChains
User->>AxelarBridgeProvider: Call getAxelarExternalUrl()
AxelarBridgeProvider->>MockAxelarAssets: Fetch asset info
AxelarBridgeProvider->>MockAxelarChains: Fetch chain info
MockAxelarAssets-->>AxelarBridgeProvider: Return asset info
MockAxelarChains-->>AxelarBridgeProvider: Return chain info
AxelarBridgeProvider-->>User: Return generated URL
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional context usedBiome
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Outside diff range and nitpick comments (3)
packages/bridge/src/skip/external-url.ts (1)
10-10: Clarify the return ofundefinedfor "testnet".Consider adding a comment explaining why
undefinedis returned when the environment is "testnet". This could help future maintainers understand the decision-making process behind this condition.packages/bridge/src/ibc/external-url.ts (1)
10-16: Clarify conditions for returningundefined.Consider adding comments to explain why
undefinedis returned for "testnet" or EVM chain types. This could help future maintainers understand the decision-making process behind these conditions.packages/bridge/src/squid/index.ts (1)
Line range hint
284-284: Ensure proper error handling for JSON parsing increateCosmosTransaction.- const parsedData = JSON.parse(data) as { + try { + const parsedData = JSON.parse(data) as { msgTypeUrl: typeof IbcTransferType | typeof WasmTransferType; msg: { sourcePort: string; sourceChannel: string; token: { denom: string; amount: string; }; sender: string; receiver: string; timeoutTimestamp: { low: number; high: number; unsigned: boolean; }; memo: string; }; + } catch (e) { + throw new BridgeQuoteError([ + { + errorType: BridgeError.CreateCosmosTxError, + message: "Failed to parse transaction data", + }, + ]); + }Adding a try-catch block around JSON parsing can prevent runtime errors from unhandled exceptions and provide clearer error messages.
| const url = new URL( | ||
| env === "mainnet" | ||
| ? "https://app.squidrouter.com/" | ||
| : "https://testnet.app.squidrouter.com/" | ||
| ); | ||
| url.searchParams.set( | ||
| "chains", | ||
| [fromChain.chainId, toChain.chainId].join(",") | ||
| ); | ||
| url.searchParams.set( | ||
| "tokens", | ||
| [fromAsset.address, toAsset.address].join(",") | ||
| ); | ||
|
|
||
| return url.toString(); |
There was a problem hiding this comment.
Ensure robust error handling in URL construction.
The URL construction logic is clear, but consider adding error handling to manage potential exceptions that might arise from malformed inputs. This could improve the robustness of the function.
| export async function getAxelarExternalUrl({ | ||
| fromChain, | ||
| toChain, | ||
| toAsset, | ||
| env, | ||
| toAddress, | ||
| fromAsset, | ||
| }: GetBridgeExternalUrlParams): Promise<string> { | ||
| const [axelarChains, axelarAssets] = await Promise.all([ | ||
| getAxelarChains({ env }), | ||
| getAxelarAssets({ env }), | ||
| ]); | ||
|
|
||
| const fromAxelarChain = axelarChains.find( | ||
| (chain) => String(chain.chain_id) === String(fromChain.chainId) | ||
| ); | ||
|
|
||
| if (!fromAxelarChain) { | ||
| throw new Error(`Chain not found: ${fromChain.chainId}`); | ||
| } | ||
|
|
||
| const toAxelarChain = axelarChains.find( | ||
| (chain) => String(chain.chain_id) === String(toChain.chainId) | ||
| ); | ||
|
|
||
| if (!toAxelarChain) { | ||
| throw new Error(`Chain not found: ${toChain.chainId}`); | ||
| } | ||
|
|
||
| const fromAxelarAsset = axelarAssets.find((axelarAsset) => { | ||
| return ( | ||
| !isNil(axelarAsset.addresses[toAxelarChain.chain_name]) && | ||
| (axelarAsset.addresses[toAxelarChain.chain_name].ibc_denom === | ||
| toAsset.address || | ||
| axelarAsset.addresses[toAxelarChain.chain_name].address === | ||
| toAsset.address) | ||
| ); | ||
| }); | ||
|
|
||
| if (!fromAxelarAsset) { | ||
| throw new Error(`Asset not found: ${toAsset.address}`); | ||
| } | ||
|
|
||
| const url = new URL( | ||
| env === "mainnet" | ||
| ? "https://satellite.money/" | ||
| : "https://testnet.satellite.money/" | ||
| ); | ||
| url.searchParams.set("source", fromAxelarChain.chain_name); | ||
| url.searchParams.set("destination", toAxelarChain.chain_name); | ||
| url.searchParams.set( | ||
| "asset_denom", | ||
| // Check if the asset has multiple denoms (indicating wrapped tokens) | ||
| !isNil(fromAxelarAsset.denoms) && fromAxelarAsset.denoms.length > 1 | ||
| ? // If the fromAsset address is the native EVM token constant address, use the second denom which is the unwrapped token | ||
| fromAsset.address === NativeEVMTokenConstantAddress | ||
| ? fromAxelarAsset.denoms[1] | ||
| : fromAxelarAsset.denoms[0] | ||
| : // If there are no multiple denoms, use the default denom | ||
| fromAxelarAsset.denom | ||
| ); | ||
| url.searchParams.set("destination_address", toAddress); | ||
|
|
||
| return url.toString(); | ||
| } |
There was a problem hiding this comment.
The function getAxelarExternalUrl is comprehensive and handles various scenarios including environment checks and asset denomination handling. Consider adding more detailed error messages to improve debugging and user feedback.
- throw new Error(`Chain not found: ${fromChain.chainId}`);
+ throw new Error(`Chain not found for ID: ${fromChain.chainId}. Please verify the chain ID and try again.`);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getAxelarExternalUrl({ | |
| fromChain, | |
| toChain, | |
| toAsset, | |
| env, | |
| toAddress, | |
| fromAsset, | |
| }: GetBridgeExternalUrlParams): Promise<string> { | |
| const [axelarChains, axelarAssets] = await Promise.all([ | |
| getAxelarChains({ env }), | |
| getAxelarAssets({ env }), | |
| ]); | |
| const fromAxelarChain = axelarChains.find( | |
| (chain) => String(chain.chain_id) === String(fromChain.chainId) | |
| ); | |
| if (!fromAxelarChain) { | |
| throw new Error(`Chain not found: ${fromChain.chainId}`); | |
| } | |
| const toAxelarChain = axelarChains.find( | |
| (chain) => String(chain.chain_id) === String(toChain.chainId) | |
| ); | |
| if (!toAxelarChain) { | |
| throw new Error(`Chain not found: ${toChain.chainId}`); | |
| } | |
| const fromAxelarAsset = axelarAssets.find((axelarAsset) => { | |
| return ( | |
| !isNil(axelarAsset.addresses[toAxelarChain.chain_name]) && | |
| (axelarAsset.addresses[toAxelarChain.chain_name].ibc_denom === | |
| toAsset.address || | |
| axelarAsset.addresses[toAxelarChain.chain_name].address === | |
| toAsset.address) | |
| ); | |
| }); | |
| if (!fromAxelarAsset) { | |
| throw new Error(`Asset not found: ${toAsset.address}`); | |
| } | |
| const url = new URL( | |
| env === "mainnet" | |
| ? "https://satellite.money/" | |
| : "https://testnet.satellite.money/" | |
| ); | |
| url.searchParams.set("source", fromAxelarChain.chain_name); | |
| url.searchParams.set("destination", toAxelarChain.chain_name); | |
| url.searchParams.set( | |
| "asset_denom", | |
| // Check if the asset has multiple denoms (indicating wrapped tokens) | |
| !isNil(fromAxelarAsset.denoms) && fromAxelarAsset.denoms.length > 1 | |
| ? // If the fromAsset address is the native EVM token constant address, use the second denom which is the unwrapped token | |
| fromAsset.address === NativeEVMTokenConstantAddress | |
| ? fromAxelarAsset.denoms[1] | |
| : fromAxelarAsset.denoms[0] | |
| : // If there are no multiple denoms, use the default denom | |
| fromAxelarAsset.denom | |
| ); | |
| url.searchParams.set("destination_address", toAddress); | |
| return url.toString(); | |
| } | |
| export async function getAxelarExternalUrl({ | |
| fromChain, | |
| toChain, | |
| toAsset, | |
| env, | |
| toAddress, | |
| fromAsset, | |
| }: GetBridgeExternalUrlParams): Promise<string> { | |
| const [axelarChains, axelarAssets] = await Promise.all([ | |
| getAxelarChains({ env }), | |
| getAxelarAssets({ env }), | |
| ]); | |
| const fromAxelarChain = axelarChains.find( | |
| (chain) => String(chain.chain_id) === String(fromChain.chainId) | |
| ); | |
| if (!fromAxelarChain) { | |
| throw new Error(`Chain not found for ID: ${fromChain.chainId}. Please verify the chain ID and try again.`); | |
| } | |
| const toAxelarChain = axelarChains.find( | |
| (chain) => String(chain.chain_id) === String(toChain.chainId) | |
| ); | |
| if (!toAxelarChain) { | |
| throw new Error(`Chain not found: ${toChain.chainId}`); | |
| } | |
| const fromAxelarAsset = axelarAssets.find((axelarAsset) => { | |
| return ( | |
| !isNil(axelarAsset.addresses[toAxelarChain.chain_name]) && | |
| (axelarAsset.addresses[toAxelarChain.chain_name].ibc_denom === | |
| toAsset.address || | |
| axelarAsset.addresses[toAxelarChain.chain_name].address === | |
| toAsset.address) | |
| ); | |
| }); | |
| if (!fromAxelarAsset) { | |
| throw new Error(`Asset not found: ${toAsset.address}`); | |
| } | |
| const url = new URL( | |
| env === "mainnet" | |
| ? "https://satellite.money/" | |
| : "https://testnet.satellite.money/" | |
| ); | |
| url.searchParams.set("source", fromAxelarChain.chain_name); | |
| url.searchParams.set("destination", toAxelarChain.chain_name); | |
| url.searchParams.set( | |
| "asset_denom", | |
| // Check if the asset has multiple denoms (indicating wrapped tokens) | |
| !isNil(fromAxelarAsset.denoms) && fromAxelarAsset.denoms.length > 1 | |
| ? // If the fromAsset address is the native EVM token constant address, use the second denom which is the unwrapped token | |
| fromAsset.address === NativeEVMTokenConstantAddress | |
| ? fromAxelarAsset.denoms[1] | |
| : fromAxelarAsset.denoms[0] | |
| : // If there are no multiple denoms, use the default denom | |
| fromAxelarAsset.denom | |
| ); | |
| url.searchParams.set("destination_address", toAddress); | |
| return url.toString(); | |
| } |
| @@ -270,7 +270,7 @@ export class SquidBridgeProvider implements BridgeProvider { | |||
| const isFromAssetNative = | |||
| fromAsset.address === NativeEVMTokenConstantAddress; | |||
| const squidFromChain = (await this.getChains()).find(({ chainId }) => { | |||
| return chainId === fromChain.chainId; | |||
| return String(chainId) === String(fromChain.chainId); | |||
There was a problem hiding this comment.
Ensure consistent string comparison for chain IDs.
- return String(chainId) === String(fromChain.chainId);
+ return chainId.toString() === fromChain.chainId.toString();This change ensures that the comparison is explicitly done between string representations, enhancing readability and maintainability.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return String(chainId) === String(fromChain.chainId); | |
| return chainId.toString() === fromChain.chainId.toString(); |
Consider adding more specific error handling in getChains.
- throw error.data;
+ if (error instanceof ApiClientError) {
+ throw new BridgeError({
+ errorType: BridgeError.NetworkError,
+ message: error.message || "Failed to fetch chain data",
+ });
+ } else {
+ throw error;
+ }This change provides more specific error handling, distinguishing between API client errors and other types of errors, which can aid in debugging and provide more informative error messages to the user.
Committable suggestion was skipped due to low confidence.
| beforeEach(() => { | ||
| server.use( | ||
| rest.get("https://api.axelarscan.io/api/getAssets", (_req, res, ctx) => { | ||
| return res(ctx.json(MockAxelarAssets)); | ||
| }), | ||
| rest.get("https://api.axelarscan.io/api/getChains", (_req, res, ctx) => { | ||
| return res(ctx.json(MockAxelarChains)); | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Optimize the setup of mock server handlers.
The repeated use of rest.get can be simplified by combining them into a single server.use call to streamline the setup.
- server.use(
- rest.get("https://api.axelarscan.io/api/getAssets", (_req, res, ctx) => {
- return res(ctx.json(MockAxelarAssets));
- }),
- rest.get("https://api.axelarscan.io/api/getChains", (_req, res, ctx) => {
- return res(ctx.json(MockAxelarChains));
- })
- );
+ server.use(
+ rest.get("https://api.axelarscan.io/api/getAssets", (_req, res, ctx) => res(ctx.json(MockAxelarAssets))),
+ rest.get("https://api.axelarscan.io/api/getChains", (_req, res, ctx) => res(ctx.json(MockAxelarChains)))
+ );Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeEach(() => { | |
| server.use( | |
| rest.get("https://api.axelarscan.io/api/getAssets", (_req, res, ctx) => { | |
| return res(ctx.json(MockAxelarAssets)); | |
| }), | |
| rest.get("https://api.axelarscan.io/api/getChains", (_req, res, ctx) => { | |
| return res(ctx.json(MockAxelarChains)); | |
| }) | |
| ); | |
| }); | |
| beforeEach(() => { | |
| server.use( | |
| rest.get("https://api.axelarscan.io/api/getAssets", (_req, res, ctx) => res(ctx.json(MockAxelarAssets))), | |
| rest.get("https://api.axelarscan.io/api/getChains", (_req, res, ctx) => res(ctx.json(MockAxelarChains))) | |
| ); | |
| }); |
| fromChain: { chainId: 1, chainName: "Ethereum", chainType: "evm" }, | ||
| toChain: { chainId: 43114, chainName: "Avalanche", chainType: "evm" }, |
There was a problem hiding this comment.
Ensure proper handling of transaction data in the test for creating an EVM transaction.
- fromChain: { chainId: 1, chainName: "Ethereum", chainType: "evm" },
- toChain: { chainId: 43114, chainName: "Avalanche", chainType: "evm" },
+ fromChain: { chainId: '1', chainName: "Ethereum", chainType: "evm" },
+ toChain: { chainId: '43114', chainName: "Avalanche", chainType: "evm" },This change ensures that chain IDs are consistently used as strings across all test cases, aligning with the updated schema definitions.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fromChain: { chainId: 1, chainName: "Ethereum", chainType: "evm" }, | |
| toChain: { chainId: 43114, chainName: "Avalanche", chainType: "evm" }, | |
| fromChain: { chainId: '1', chainName: "Ethereum", chainType: "evm" }, | |
| toChain: { chainId: '43114', chainName: "Avalanche", chainType: "evm" }, |
| chainId: 989898989898, | ||
| chainName: "Unsupported", | ||
| chainType: "evm", | ||
| networkName: "unsupported", | ||
| }, | ||
| toChain: { | ||
| chainId: "unsupported", | ||
| chainId: 989898989898, | ||
| chainName: "Unsupported", | ||
| chainType: "evm", | ||
| networkName: "unsupported", |
There was a problem hiding this comment.
Refine the error message for better clarity in the test for unsupported chains.
- }).rejects.toThrow("Unsupported chain");
+ }).rejects.toThrow("Unsupported chain: Chain ID 989898989898 is not supported.");This change provides a more descriptive error message, which can help in debugging and understanding the specific cause of the error.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chainId: 989898989898, | |
| chainName: "Unsupported", | |
| chainType: "evm", | |
| networkName: "unsupported", | |
| }, | |
| toChain: { | |
| chainId: "unsupported", | |
| chainId: 989898989898, | |
| chainName: "Unsupported", | |
| chainType: "evm", | |
| networkName: "unsupported", | |
| chainId: 989898989898, | |
| chainName: "Unsupported", | |
| chainType: "evm", | |
| networkName: "unsupported", | |
| }, | |
| toChain: { | |
| chainId: 989898989898, | |
| chainName: "Unsupported", | |
| chainType: "evm", | |
| networkName: "unsupported", | |
| ``` | |
| }).rejects.toThrow("Unsupported chain: Chain ID 989898989898 is not supported."); |
| fromChain: { chainId: 1, chainName: "Ethereum", chainType: "evm" }, | ||
| toChain: { chainId: 43114, chainName: "Avalanche", chainType: "evm" }, |
There was a problem hiding this comment.
Improve error handling in the test for getQuote.
- }).rejects.toThrow("Query client error");
+ }).rejects.toThrow(new Error("Query client error"));This change ensures that errors are properly instantiated and thrown, which can help in debugging and understanding the specific cause of the error.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fromChain: { chainId: 1, chainName: "Ethereum", chainType: "evm" }, | |
| toChain: { chainId: 43114, chainName: "Avalanche", chainType: "evm" }, | |
| fromChain: { chainId: 1, chainName: "Ethereum", chainType: "evm" }, | |
| toChain: { chainId: 43114, chainName: "Avalanche", chainType: "evm" }, | |
| // Assuming the context of the test case is provided here | |
| await expect(getQuote()).rejects.toThrow(new Error("Query client error")); |
| fromChain: { chainId: 1, chainName: "Ethereum", chainType: "evm" }, | ||
| toChain: { chainId: 43114, chainName: "Avalanche", chainType: "evm" }, |
There was a problem hiding this comment.
Ensure consistent use of numeric chain IDs in test cases.
- fromChain: { chainId: 1, chainName: "Ethereum", chainType: "evm" },
- toChain: { chainId: 43114, chainName: "Avalanche", chainType: "evm" },
+ fromChain: { chainId: '1', chainName: "Ethereum", chainType: "evm" },
+ toChain: { chainId: '43114', chainName: "Avalanche", chainType: "evm" },This change ensures that chain IDs are consistently used as strings across all test cases, aligning with the updated schema definitions.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fromChain: { chainId: 1, chainName: "Ethereum", chainType: "evm" }, | |
| toChain: { chainId: 43114, chainName: "Avalanche", chainType: "evm" }, | |
| fromChain: { chainId: '1', chainName: "Ethereum", chainType: "evm" }, | |
| toChain: { chainId: '43114', chainName: "Avalanche", chainType: "evm" }, |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
packages/bridge/src/axelar/__tests__/axelar-bridge-provider.spec.ts (1)
Line range hint
17-26: Consider using arrow functions for better readability and consistency.- jest.mock("viem", function () { + jest.mock("viem", () => { return { ...jest.requireActual("viem"), createPublicClient: jest.fn().mockImplementation(() => ({ estimateGas: jest.fn().mockResolvedValue(BigInt("21000")), getGasPrice: jest.fn().mockResolvedValue(BigInt("0x4a817c800")), })), http: jest.fn(), }; - }); + });packages/bridge/src/axelar/index.ts (2)
Line range hint
311-313: Consider omitting the else clause as previous branches break early, improving code readability.- } else { + }
Line range hint
365-375: Consider omitting the else clause as previous branches break early, improving code readability.- } else { + }
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
packages/bridge/src/axelar/index.ts (1)
Line range hint
311-313: Consider removing unnecessary else clauses.The static analysis tool has identified unnecessary else clauses that can be removed to simplify the code. If these clauses are indeed redundant, removing them could enhance code readability and maintainability.
Also applies to: 365-375
| throw new Error( | ||
| `Unsupported chain: Chain ID ${ | ||
| !fromChainAxelarId ? fromChain.chainId : toChain.chainId | ||
| } is not supported.` | ||
| ); |
There was a problem hiding this comment.
Refine the error handling for unsupported chains.
The error message construction can be optimized by directly using template literals for the entire message, which enhances readability and maintainability:
- throw new Error(
- `Unsupported chain: Chain ID ${
- !fromChainAxelarId ? fromChain.chainId : toChain.chainId
- } is not supported.`
- );
+ throw new Error(`Unsupported chain: Chain ID ${fromChainAxelarId ? toChain.chainId : fromChain.chainId} is not supported.`);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new Error( | |
| `Unsupported chain: Chain ID ${ | |
| !fromChainAxelarId ? fromChain.chainId : toChain.chainId | |
| } is not supported.` | |
| ); | |
| throw new Error(`Unsupported chain: Chain ID ${fromChainAxelarId ? toChain.chainId : fromChain.chainId} is not supported.`); |
What is the purpose of the change:
Providers often have their own user interfaces (UIs) that support their unique IDs, such as Satellite for Axelar. We could query each provider using the counterparty chain and token to obtain a URL that preselects those assets, along with Osmosis as the source or destination.
Linear Task
Linear Task URL
Testing and Verifying