Add can-transfer check #4078
Add can-transfer check #4078
Conversation
|
@artakmik-nitka is attempting to deploy a commit to the OsmoLabs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes introduce a new validation step in the bridge quote process to ensure that asset transfers between chains are permitted before proceeding. This is accomplished by implementing a new function that queries transfer availability from an external API, using chain and asset identifiers. The quote method now calls this function and throws a specific error if transfers are not allowed, using the API's response for error messaging. Supporting types and mappings for asset denominations are also added to facilitate this check. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Int3faceBridgeProvider
participant checkCanTransfer (API)
User->>Int3faceBridgeProvider: getQuote(params)
Int3faceBridgeProvider->>checkCanTransfer (API): checkCanTransfer(fromChain, toChain, assetId, env)
checkCanTransfer (API)-->>Int3faceBridgeProvider: {can_transfer, reason}
alt can_transfer is true
Int3faceBridgeProvider-->>User: Return bridge quote
else can_transfer is false/undefined
Int3faceBridgeProvider-->>User: Throw BridgeQuoteError with reason
end
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 3
🧹 Nitpick comments (1)
packages/bridge/src/int3face/index.ts (1)
54-78: Consider extracting the transfer validation logicThe
getQuotemethod is quite complex and the added validation increases its complexity. Consider extracting this logic into a separate method for better maintainability.+ private async validateTransfer(fromChain: BridgeChain, toChain: BridgeChain, fromAsset: BridgeAsset): Promise<void> { + if (toChain.chainId !== "dogecoin") { + throw new BridgeQuoteError({ + bridgeId: Int3faceProviderId, + errorType: "UnsupportedQuoteError", + message: "Only Dogecoin is supported as a destination chain.", + }); + } + + // Check if transfer is available on Int3face side + const canTransfer = await checkCanTransfer( + fromChain.chainId, + toChain.chainId, + fromAsset.denom, + this.ctx.env + ); + + if (!canTransfer || canTransfer.can_transfer === false) { + throw new BridgeQuoteError({ + bridgeId: Int3faceProviderId, + errorType: "UnsupportedQuoteError", + message: canTransfer?.reason || "Transfer verification failed or is not available at this time", + }); + } + } async getQuote(params: GetBridgeQuoteParams): Promise<BridgeQuote> { const { fromAddress, fromChain, toChain, toAddress, fromAsset, fromAmount } = params; - if (toChain.chainId !== "dogecoin") { - throw new BridgeQuoteError({ - bridgeId: Int3faceProviderId, - errorType: "UnsupportedQuoteError", - message: "Only Dogecoin is supported as a destination chain.", - }); - } - - // Check if transfer is available on Int3face side - const canTransfer = await checkCanTransfer( - fromChain.chainId, - toChain.chainId, - fromAsset.denom, - this.ctx.env - ); - - if (!canTransfer?.can_transfer) { - throw new BridgeQuoteError({ - bridgeId: Int3faceProviderId, - errorType: "UnsupportedQuoteError", - message: canTransfer?.reason || "Transfer is not available at this time", - }); - } + await this.validateTransfer(fromChain, toChain, fromAsset);This refactoring separates concerns, making the code more maintainable and easier to test. The
validateTransfermethod handles all the validation logic whilegetQuotefocuses on its primary responsibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bridge/src/int3face/index.ts(3 hunks)packages/bridge/src/int3face/queries.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/bridge/src/int3face/index.ts (3)
packages/bridge/src/int3face/queries.ts (1)
checkCanTransfer(54-75)packages/bridge/src/errors.ts (1)
BridgeQuoteError(2-18)packages/bridge/src/int3face/utils.ts (1)
Int3faceProviderId(1-1)
packages/bridge/src/int3face/queries.ts (1)
packages/utils/src/api-client.ts (1)
apiClient(54-148)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
packages/bridge/src/int3face/queries.ts (1)
45-48: Well-structured interface for API responseThis interface appropriately represents the response from the can-transfer API with proper typing for both the boolean flag and the reason message.
packages/bridge/src/int3face/index.ts (3)
24-24: Correctly imports the new functionThe import statement is properly added for the new
checkCanTransferfunction.
54-54: Proper parameter destructuring to support new featureThe code now correctly destructures
fromChainfrom the parameters to use in the transfer check.
65-70: Implementation of the can-transfer validation checkThe implementation correctly uses the newly added
checkCanTransferfunction to validate whether the transfer is allowed before proceeding with quote generation.
| export async function checkCanTransfer( | ||
| srcChainId: string | number, | ||
| destChainId: string, | ||
| assetId: string, | ||
| env: "testnet" | "mainnet" | ||
| ): Promise<CanTransferResponse> { | ||
| let srcChainIdConverted; | ||
|
|
||
| if (typeof srcChainId === "string" && srcChainId.startsWith("osmosis")) { | ||
| srcChainIdConverted = "osmosis" | ||
| } else { | ||
| srcChainIdConverted = srcChainId.toString(); | ||
| } | ||
|
|
||
| const origin = env === "mainnet" | ||
| ? "https://api.mainnet.int3face.zone/int3face" | ||
| : "https://api.testnet.int3face.zone/int3face"; | ||
|
|
||
| const url = new URL(`/bridge/v1beta1/can-transfer/${srcChainIdConverted}/${destChainId}/${denomOfInt3face[assetId]}`, origin); | ||
|
|
||
| return apiClient<CanTransferResponse>(url.toString()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and use the safer denom lookup method
The function needs to handle possible API errors and provide better context for debugging. Also, it should use a safer method to get the denom to avoid potential undefined values in the URL.
export async function checkCanTransfer(
srcChainId: string | number,
destChainId: string,
assetId: string,
env: "testnet" | "mainnet"
): Promise<CanTransferResponse> {
let srcChainIdConverted;
if (typeof srcChainId === "string" && srcChainId.startsWith("osmosis")) {
srcChainIdConverted = "osmosis"
} else {
srcChainIdConverted = srcChainId.toString();
}
const origin = env === "mainnet"
? "https://api.mainnet.int3face.zone/int3face"
: "https://api.testnet.int3face.zone/int3face";
- const url = new URL(`/bridge/v1beta1/can-transfer/${srcChainIdConverted}/${destChainId}/${denomOfInt3face[assetId]}`, origin);
+ const denom = getInt3faceDenom(assetId);
+ const url = new URL(`/bridge/v1beta1/can-transfer/${srcChainIdConverted}/${destChainId}/${denom}`, origin);
- return apiClient<CanTransferResponse>(url.toString());
+ try {
+ return await apiClient<CanTransferResponse>(url.toString());
+ } catch (error) {
+ console.error(`Failed to check transfer availability: ${error}`);
+ // Rethrow the error to be handled by the caller
+ throw error;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| const denomOfInt3face: Record<string, string> = { | ||
| DOGE: 'dogecoin-doge' | ||
| } |
There was a problem hiding this comment.
Add error handling for unsupported assets
The mapping only contains one entry for DOGE. When this mapping is used in checkCanTransfer, there's no validation to ensure the assetId exists in the mapping, which could lead to undefined values in the API URL.
const denomOfInt3face: Record<string, string> = {
DOGE: 'dogecoin-doge'
}
+
+export function getInt3faceDenom(assetId: string): string {
+ const denom = denomOfInt3face[assetId];
+ if (!denom) {
+ throw new Error(`Unsupported asset ID: ${assetId}`);
+ }
+ return denom;
+}📝 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.
| const denomOfInt3face: Record<string, string> = { | |
| DOGE: 'dogecoin-doge' | |
| } | |
| const denomOfInt3face: Record<string, string> = { | |
| DOGE: 'dogecoin-doge' | |
| } | |
| export function getInt3faceDenom(assetId: string): string { | |
| const denom = denomOfInt3face[assetId]; | |
| if (!denom) { | |
| throw new Error(`Unsupported asset ID: ${assetId}`); | |
| } | |
| return denom; | |
| } |
| if (!canTransfer?.can_transfer) { | ||
| throw new BridgeQuoteError({ | ||
| bridgeId: Int3faceProviderId, | ||
| errorType: "UnsupportedQuoteError", | ||
| message: canTransfer?.reason || "Transfer is not available at this time", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve null handling in the can-transfer check
The current check may not handle all edge cases properly. It's better to be more explicit about the conditions that should trigger the error.
- if (!canTransfer?.can_transfer) {
+ if (!canTransfer || canTransfer.can_transfer === false) {
throw new BridgeQuoteError({
bridgeId: Int3faceProviderId,
errorType: "UnsupportedQuoteError",
- message: canTransfer?.reason || "Transfer is not available at this time",
+ message: canTransfer?.reason || "Transfer verification failed or is not available at this time",
});
}This change makes the check more explicit by verifying both that the response exists and that the can_transfer property is specifically false. It also provides a more descriptive message when the reason is empty or when canTransfer is null.
📝 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.
| if (!canTransfer?.can_transfer) { | |
| throw new BridgeQuoteError({ | |
| bridgeId: Int3faceProviderId, | |
| errorType: "UnsupportedQuoteError", | |
| message: canTransfer?.reason || "Transfer is not available at this time", | |
| }); | |
| } | |
| if (!canTransfer || canTransfer.can_transfer === false) { | |
| throw new BridgeQuoteError({ | |
| bridgeId: Int3faceProviderId, | |
| errorType: "UnsupportedQuoteError", | |
| message: canTransfer?.reason || "Transfer verification failed or is not available at this time", | |
| }); | |
| } |
What is the purpose of the change:
Linear Task
Brief Changelog
Testing and Verifying