feat: token fees quote #746
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
4 Skipped Deployments
|
There was a problem hiding this comment.
for maxAmount, I still need to figure out if I should call the getLowestFeeRoute here, since it might affect the max amount of the warp route (answer is probably yes)
📝 WalkthroughWalkthroughSwitches dependencies to local Hyperlane monorepo paths, tightens warp route defaults, populates warpRoutes with USDC/ETH configs, and introduces a unified fee-quote system: new helpers, hook, tests, UI button and modal. Transfer form is refactored to use lowest-fee routing, new quotes, and updated validation/sender handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Form as TransferTokenForm
participant Hook as useFeeQuotes
participant Fees as fees.ts helpers
participant Core as WarpCore
participant Wallet as Wallet/Signer
User->>Form: Enter amount, recipient
Form->>Hook: useFeeQuotes(destination, amount, recipient, originToken, searchForLowestFee)
Hook->>Fees: getLowestFeeTransferToken(...)? (when enabled)
Fees->>Core: estimate token fee per candidate
Core-->>Fees: tokenFeeQuote(s)
Fees-->>Hook: chosen transferToken
Hook->>Core: estimateTransferRemoteFees(originTokenAmount, recipient, senderPubKey)
Core-->>Hook: WarpCoreFeeEstimate { interchainQuote, localQuote, tokenFeeQuote? }
Hook-->>Form: fees, isLoading
Form->>Form: compute total via getTotalFee/getInterchainQuote
User->>Form: Click "Fees" button
Form->>Form: Open TransferFeeModal
Note over Form: Modal shows Local Gas / Interchain Gas / Token Fee
sequenceDiagram
autonumber
actor User
participant Form as TransferTokenForm
participant Fees as fees.ts helpers
participant Core as WarpCore
participant Wallet as Wallet/Signer
User->>Form: Submit transfer (Review)
Form->>Wallet: get address + pubkey (by origin chain)
Form->>Fees: getLowestFeeTransferToken(warpCore, originToken, destinationToken, amount, recipient, sender)
Fees->>Core: estimate token fee per route
Core-->>Fees: tokenFeeQuote results
Fees-->>Form: selected transferToken
Form->>Core: validateTransfer({ sender, senderPubKey, ... })
Core-->>Form: validation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/features/transfer/TransferTokenForm.tsx (2)
741-759: Multi-collateral limit uses the wrong decimals.You compute
amountWeiusingtransferToken.decimalsbut pass it to a limit check for the originally selectedtoken. If their decimals differ, the comparison is off and can block/allow transfers incorrectly.- const amountWei = toWei(amount, transferToken.decimals); - const multiCollateralLimit = isMultiCollateralLimitExceeded(token, destination, amountWei); + const amountWeiForTransfer = toWei(amount, transferToken.decimals); + const amountWeiForSelectedToken = toWei(amount, token.decimals); + const multiCollateralLimit = isMultiCollateralLimitExceeded( + token, + destination, + amountWeiForSelectedToken, + ); @@ - const result = await warpCore.validateTransfer({ - originTokenAmount: transferToken.amount(amountWei), + const result = await warpCore.validateTransfer({ + originTokenAmount: transferToken.amount(amountWeiForTransfer),
473-479: Fix user-facing typo in warning.Double “for” and a small punctuation tweak.
- toast.warn(`No account found for for chain ${chainDisplayName}, is your wallet connected?`); + toast.warn(`No account found for chain ${chainDisplayName}. Is your wallet connected?`);
🧹 Nitpick comments (14)
src/consts/warpRouteWhitelist.ts (1)
5-5: Empty whitelist blocks all registry routes—confirm this is intendedSetting the default to [] means no registry routes will load unless they’re explicitly listed. If the goal was “include all unless specified,” keep this at null or add a clarifying note.
Apply this small doc tweak if you keep []:
-// If left null, all warp routes in the configured registry will be included -// If set to a list (including an empty list), only the specified routes will be included +// If left null, all warp routes in the configured registry will be included +// If set to a list (including an empty list), only the specified routes will be included. +// NOTE: [] excludes all registry routes; only routes defined in warpRoutes.yaml will appear.src/features/transfer/maxAmount.ts (1)
55-55: Tighten log messageMinor wording nit: this path isn’t fetching “fee quotes.”
- logger.warn('Error fetching fee quotes for max amount', error); + logger.warn('Error fetching max transfer amount', error);src/features/transfer/fees.test.ts (2)
14-19: Make fungibility mocking symmetric to avoid order flakinessOnly token1.isFungibleWith is stubbed, but the code calls token2.isFungibleWith(token1) on the second pass. Stub both to keep the test deterministic.
- vi.spyOn(token1, 'isFungibleWith').mockReturnValue(true); + vi.spyOn(token1, 'isFungibleWith').mockReturnValue(true); + vi.spyOn(token2, 'isFungibleWith').mockReturnValue(true);
1-3: Add coverage for getLowestFeeTransferToken comparatorGiven routing now hinges on the lowest tokenFeeQuote, add a focused test that feeds two candidate routes with different tokenFeeQuote amounts and asserts the cheaper token is chosen. This guards the sort logic.
Happy to sketch the test with a mocked WarpCore.getInterchainTransferFee if you want.
src/features/transfer/FeeSectionButton.tsx (1)
25-33: Accessibility touch-up for the fees buttonAdd an aria-label so screen readers aren’t left guessing, and consider a title tooltip for parity with the modal.
- <button + <button className="flex w-fit items-center text-xxs text-gray-600 hover:text-gray-500 [&_path]:fill-gray-600 [&_path]:hover:fill-gray-500" type="button" onClick={open} + aria-label="View fee details" + title={`Fees: ${fees.totalFees}`} >src/features/transfer/TransferFeeModal.tsx (1)
15-16: Avoid rendering an empty modalIf there are no fees and we’re not loading, bail early to prevent a blank panel with just a title.
- // if (!fees) return null; + if (!isLoading && !fees) return null;src/features/transfer/fees.ts (3)
50-53: Minor readability: use ‘in’ check for rent estimateFunctionally fine; this reads a tad clearer.
- return originToken && objKeys(chainsRentEstimate).includes(originToken.chainName) - ? interchainQuote.plus(chainsRentEstimate[originToken.chainName]) + return originToken && originToken.chainName in chainsRentEstimate + ? interchainQuote.plus( + chainsRentEstimate[originToken.chainName as keyof typeof chainsRentEstimate], + ) : interchainQuote;
122-124: Only log when we actually switch routesRight now we’ll log a switch even if the origin token remains best.
- logger.debug('Found route with lower fee, switching route...'); - return tokenFees[0].token; + const best = tokenFees[0].token; + if (best.addressOrDenom !== originToken.addressOrDenom) { + logger.debug('Found route with lower fee, switching route...'); + } + return best;
59-65: Confirm product intent: compare only token feesWe’re optimizing solely for tokenFeeQuote. If local or interchain gas differs between candidates, total cost might be higher despite a lower token fee. If the goal is “lowest total fees,” we should compare summed fees instead.
I can wire a compare-by-total variant if that’s the desired behavior.
src/features/transfer/useFeeQuotes.ts (4)
11-11: Comment says 10s, value is 15s.Tiny mismatch—pick one to avoid confusion.
-const FEE_QUOTE_REFRESH_INTERVAL = 15_000; // 10s +const FEE_QUOTE_REFRESH_INTERVAL = 15_000; // 15s
23-23: Use the same blacklist source when deriving accounts.Quotes should respect the same account filtering as the form. Passing the blacklist keeps behavior consistent.
- const { accounts } = useAccounts(multiProvider); + const { accounts } = useAccounts(multiProvider, config.addressBlacklist);Add import:
import { logger } from '../../utils/logger'; +import { config } from '../../consts/config';
73-74: Remove duplicate guard check.You’re checking
!originTokentwice.- if (!originToken || !destination || !sender || !originToken || !amount || !recipient) return null; + if (!originToken || !destination || !sender || !amount || !recipient) return null;
33-58: Optional: tame refetch jitter while typing.You already debounce amount; adding
staleTimemakes the UI feel smoother and cuts extra network chatter.const { isLoading, isError, data, isFetching } = useQuery({ @@ - refetchInterval: FEE_QUOTE_REFRESH_INTERVAL, + refetchInterval: FEE_QUOTE_REFRESH_INTERVAL, + staleTime: 5_000, });src/features/transfer/TransferTokenForm.tsx (1)
523-528: Show received amount in destination units.You’re converting and labeling with the origin token’s decimals/symbol. For clarity, format in destination token units.
- value: fromWei(value.toString(), originToken.decimals), + value: fromWei(value.toString(), destinationToken.decimals),- <span>{`${scaledAmount.value} ${originTokenSymbol} (scaled from ${scaledAmount.originScale} to ${scaledAmount.destinationScale})`}</span> + <span>{`${scaledAmount.value} ${destinationToken?.symbol || originTokenSymbol} (scaled from ${scaledAmount.originScale} to ${scaledAmount.destinationScale})`}</span>Also applies to: 611-613
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
package.json(2 hunks)src/consts/warpRouteWhitelist.ts(1 hunks)src/consts/warpRoutes.yaml(1 hunks)src/features/transfer/FeeSectionButton.tsx(1 hunks)src/features/transfer/TransferFeeModal.tsx(1 hunks)src/features/transfer/TransferTokenForm.tsx(10 hunks)src/features/transfer/fees.test.ts(1 hunks)src/features/transfer/fees.ts(1 hunks)src/features/transfer/maxAmount.ts(1 hunks)src/features/transfer/useFeeQuotes.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-17T15:50:57.645Z
Learnt from: Xaroz
PR: hyperlane-xyz/hyperlane-warp-ui-template#673
File: src/features/transfer/TransferTokenForm.tsx:498-518
Timestamp: 2025-07-17T15:50:57.645Z
Learning: In the Hyperlane Warp UI codebase, when calculating scaled amounts for token transfers, converting to Wei first using `toWei(amount, originToken.decimals)` is considered sufficient for precision handling, and additional error handling for BigInt operations in the scaling calculation is not required according to the maintainer.
Applied to files:
src/features/transfer/TransferTokenForm.tsx
🧬 Code graph analysis (5)
src/features/transfer/FeeSectionButton.tsx (2)
src/styles/Color.ts (1)
Color(6-13)src/features/transfer/TransferFeeModal.tsx (1)
TransferFeeModal(4-72)
src/features/transfer/fees.test.ts (2)
src/utils/test.ts (1)
createMockToken(28-30)src/features/transfer/fees.ts (1)
getTotalFee(7-42)
src/features/transfer/fees.ts (3)
src/consts/chains.ts (1)
chainsRentEstimate(68-73)src/features/tokens/utils.ts (2)
isValidMultiCollateralToken(44-67)getTokensWithSameCollateralAddresses(69-112)src/utils/logger.ts (1)
logger(5-18)
src/features/transfer/useFeeQuotes.ts (5)
src/features/transfer/types.ts (1)
TransferFormValues(1-7)src/features/chains/hooks.ts (1)
useMultiProvider(4-6)src/features/tokens/hooks.ts (1)
useWarpCore(9-11)src/features/transfer/fees.ts (1)
getLowestFeeTransferToken(58-124)src/utils/logger.ts (1)
logger(5-18)
src/features/transfer/TransferTokenForm.tsx (4)
src/features/tokens/approval.ts (1)
useIsApproveRequired(8-28)src/features/transfer/useFeeQuotes.ts (1)
useFeeQuotes(13-61)src/features/transfer/fees.ts (3)
getInterchainQuote(44-53)getTotalFee(7-42)getLowestFeeTransferToken(58-124)src/features/transfer/FeeSectionButton.tsx (1)
FeeSectionButton(6-39)
🔇 Additional comments (3)
src/features/transfer/maxAmount.ts (1)
42-44: Recipient defaults to sender—confirm SDK expectationPassing recipient: address is fine, but please confirm WarpCore.getMaxTransferAmount uses recipient in its simulation and that this matches product intent.
src/consts/warpRoutes.yaml (1)
5-55: Sanity-check passed — warpRoutes.yaml connections are reciprocal & uniqueInspected src/consts/warpRoutes.yaml: every token’s connections have matching reciprocal entries, no duplicate (chainName,addressOrDenom) pairs, USDC decimals=6 and ETH decimals=18 — all tidy.
src/features/transfer/TransferTokenForm.tsx (1)
537-542: Minor: confirm intent to search lowest-fee only pre-review.You pass
!isReviewto the hook, so post-review you’ll stop searching the cheapest multi-collateral route. If that’s intentional to “lock” the route for review, all good; otherwise wire a state that freezes the chosen token while still allowing quote refreshes.Would you like me to wire a “lockedTransferToken” state so the pre-review choice persists through review while continuing to refresh quotes?
| "@hyperlane-xyz/sdk": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/typescript/sdk", | ||
| "@hyperlane-xyz/utils": "16.2.0", | ||
| "@hyperlane-xyz/widgets": "16.2.0", | ||
| "@hyperlane-xyz/widgets": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/typescript/widgets", | ||
| "@interchain-ui/react": "^1.23.28", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Absolute file: paths will break CI and teammates
The local overrides under your home directory will fail installs off your machine and likely in CI. Use published versions, workspaces, or relative links during local dev only.
Recommended quick fix before merge (restore published versions and drop the absolute resolutions):
- "@hyperlane-xyz/sdk": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/typescript/sdk",
+ "@hyperlane-xyz/sdk": "16.2.0",
- "@hyperlane-xyz/widgets": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/typescript/widgets",
+ "@hyperlane-xyz/widgets": "16.2.0",And remove absolute file: resolutions (keep the starknetkit patch):
- "@hyperlane-xyz/core": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/solidity",
- "@hyperlane-xyz/cosmos-sdk": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/typescript/cosmos-sdk",
- "@hyperlane-xyz/cosmos-types": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/typescript/cosmos-types",
- "@hyperlane-xyz/starknet-core": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/starknet",
- "@hyperlane-xyz/sdk": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/typescript/sdk",
- "@hyperlane-xyz/utils": "file:/Users/jasonguo/Documents/hyperlane/hyperlane-monorepo/typescript/utils"If you need local dev against the monorepo, prefer:
- Yarn workspaces (workspace:*),
- portal:/link: with relative paths, or
- a dev-only .yarnrc.yml with overrides not committed.
Run this to catch any lingering absolute file deps:
Also applies to: 112-119
🏁 Script executed:
#!/bin/bash
rg -nP '"file:(/|[A-Za-z]:\\)' package.jsonLength of output: 903
Remove absolute "file:" dependencies from package.json
Local "file:" paths will break CI and other devs' installs — replace with published versions or workspace/relative links.
Locations: package.json — lines 21, 23, 113–118.
Quick actions:
- Replace entries at lines 21 and 23 with published versions or workspace:* (example: "@hyperlane-xyz/sdk": "16.2.0", "@hyperlane-xyz/widgets": "16.2.0").
- Remove or replace the file: entries in the resolutions block (lines 113–118); keep only intentional local patches in a non-committed dev config.
🤖 Prompt for AI Agents
In package.json around lines 21–24 (and in the resolutions block around lines
113–118), remove the absolute "file:" dependencies and replace them with
published versions or workspace-relative references; specifically change
"@hyperlane-xyz/sdk": "file:.../sdk" and "@hyperlane-xyz/widgets":
"file:.../widgets" to published versions (e.g. "16.2.0") or "workspace:*" if you
intend monorepo linking, and delete any "file:" entries in the resolutions block
so only intentional published or workspace overrides remain; ensure package.json
remains valid JSON and update lockfile (npm/yarn/pnpm) after making the changes.
There was a problem hiding this comment.
will do after monorepo is published
There was a problem hiding this comment.
@Xaroz, sounds good! That makes perfect sense - you'll need those local file references to test against the latest monorepo changes until the new versions are published. Just don't forget to swap 'em back to the published versions before merging!
✏️ Learnings added
Learnt from: Xaroz
PR: hyperlane-xyz/hyperlane-warp-ui-template#746
File: package.json:21-24
Timestamp: 2025-09-12T14:39:07.851Z
Learning: In the hyperlane-warp-ui-template repository, developers may temporarily use local file: paths in package.json to test against unpublished monorepo changes, but these should be reverted to published versions before merging.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # These configs will be merged with the warp routes in the configured registry | ||
| # The input here is typically the output of the Hyperlane CLI warp deploy command | ||
| --- | ||
| tokens: [] |
There was a problem hiding this comment.
i think this was leftover too
| for (let i = 0; i < feeGroups.length; i++) { | ||
| if (tokenAmount.token.isFungibleWith(feeGroups[i].token)) { | ||
| feeGroups[i] = feeGroups[i].plus(tokenAmount.amount); | ||
| foundFungibleGroup = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
it is kinda hard to understand what is going on here
|
@xeno097 most of the dummy stuff I'll remove/update once we have the monorepo updated |
feat: multi-collateral warp routes maxTransfer
| {isLoading ? ( | ||
| <Skeleton className="h-4 w-52" /> | ||
| ) : ( | ||
| <span>{`${fees.localQuote.getDecimalFormattedAmount().toFixed(8) || '0'} ${ |
There was a problem hiding this comment.
I see the reference of toFixed(8) in multiple places. why is this used and should we store it as a const somwhere?
There was a problem hiding this comment.
There are cases where there could be a huge amount of decimals (18) and that wouldn't fit in the display space so we set it to 8 decimals
We don't need to save it as a const because for each type of fee we do toFixed(8) and it's mostly a display thing and we don't want to affect the actual value by storing it.
This PR is taken from #746, removing only the piece related to tokenFeeQuote and getting lowest fee route - Adds FeeSectionButton and TransferFeeModal that displays the amount of transfer fee the user has to pay - Update useFeeQuotes to reflect the changes related to fees - Update hyperlane packages <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a fee section button and a modal showing detailed local and interchain fee breakdowns. * Added utilities to compute and consolidate transfer fees and to select the optimal transfer token. * **Updates** * Updated Hyperlane registry, SDK, utils, and widgets to newer versions. * Added a documentation link for fee estimation. * Review/details view updated to conditionally surface fee information and improved fee-fetching behavior (optional highest-collateral token search). * **Tests** * Added comprehensive tests covering fee aggregation and edge cases. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR closes ENG-1979, ENG-2078 and ENG-2079
With the inclusion of
tokenFeeQuote, the UI needs to account for this new fee fieldFeeSectionButtonthat shows total fees, this section will show before validation and clicking it will show a detail modalSummary by CodeRabbit
New Features
Configuration
Refactor
Tests