Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR extends signature verification across chain adapters to accept an Changes
Sequence DiagramsequenceDiagram
participant User as Frontend App
participant Builder as Stellar Utils
participant Service as StellarService
participant Verify as verifyOrderSignature
User->>Builder: buildStellarUnlockMessage(orderParams)
Builder-->>User: JSON message string
User->>User: SHA256 digest of "Stellar Signed Message:\n" + message
User->>User: Sign digest with ed25519 keypair
User->>Service: confirmUnlockFunds(tradeId, signature)
Service->>Service: extract orderParams from trade
Service->>Verify: verifyOrderSignature(address, orderHash, orderParams, signature)
Verify->>Builder: buildStellarUnlockMessage(orderParams)
Builder-->>Verify: JSON message string
Verify->>Verify: stellarSignedMessageDigest(message)
Verify->>Verify: verifyEd25519(digest, signature)
Verify-->>Service: boolean result
Service-->>User: confirmation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/frontend/utils/wagmi-config.ts (1)
2-20:⚠️ Potential issue | 🟠 MajorRemove Hedera references or re-add to wagmi config.
hederaTestnetis removed fromchainListandtransportsin wagmi-config.ts, but still referenced inapps/frontend/lib/chain-icons.tsand hardcoded inapps/frontend/components/dashboard/AdCard.tsx:21-23(explorer_urls). If the backend returns ads with a Hedera chainId, they will render in AdCard with working explorer links, butwriteContractAsynccalls for withdraw/top-up/close will fail since the chain has no transport configured.Either remove Hedera from chain-icons.ts and AdCard's explorer_urls, or re-add it to wagmi-config with a transport to maintain full support.
Fix to restore Hedera support
-import { foundry, sepolia, type Chain } from "viem/chains" +import { foundry, hederaTestnet, sepolia, type Chain } from "viem/chains" const chainList: readonly [Chain, ...Chain[]] = localEnabled - ? [foundry, sepolia] - : [sepolia] + ? [foundry, sepolia, hederaTestnet] + : [sepolia, hederaTestnet] export const config = createConfig({ chains: chainList, transports: { [foundry.id]: http(anvilRpc), [sepolia.id]: http(), + [hederaTestnet.id]: http(), }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/utils/wagmi-config.ts` around lines 2 - 20, The wagmi config removed hederaTestnet but other code (chain-icons.ts and AdCard.tsx's explorer_urls) still expect it, causing writeContractAsync calls to fail without a transport; either fully remove Hedera references or re-add it to the wagmi configuration. Fix option A: remove hederaTestnet usages by deleting Hedera entries from chain-icons.ts and remove/guard explorer_urls in AdCard.tsx (references to explorer_urls and any hedera chainId checks). Fix option B: re-add hederaTestnet into the chainList and transports passed to createConfig (ensure you import the hederaTestnet Chain, add it to chainList when appropriate, and provide a corresponding http transport entry keyed by hederaTestnet.id) so writeContractAsync has a transport; pick one approach and make the code changes consistently across chain-icons.ts, AdCard.tsx (explorer_urls), and wagmi-config's createConfig.apps/backend-relayer/src/modules/trades/trade.service.ts (2)
1051-1051:⚠️ Potential issue | 🟡 MinorRemove stray
console.log(e)from catch block.This dumps full error objects (possibly including signatures, addresses, stack traces with internals) to stdout in production. Prefer the framework
Loggeratwarn/errorlevel, or drop it now that behavior is stable.🧹 Proposed fix
} catch (e) { - console.log(e); if (e instanceof Error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/src/modules/trades/trade.service.ts` at line 1051, Stray console.log(e) in the catch block of TradeService should be removed and replaced with the framework logger; locate the catch in apps/backend-relayer/src/modules/trades/trade.service.ts (class TradeService / the method containing the shown catch) and either drop the console.log(e) entirely or call the project's logger at the appropriate level (e.g. this.logger.error('Descriptive message', e) or Logger.error('Descriptive message', e, 'TradeService')) so sensitive error objects are not printed to stdout — ensure you use the existing logger instance/name used elsewhere in TradeService for consistency.
1355-1355:⚠️ Potential issue | 🟡 MinorLeftover debug
console.log(dto)inconfirmUnlockChainAction.
dtocontainstxHashand (optionally)signature— neither catastrophic, but this is clearly a debugging artifact and should be removed before merge.🧹 Proposed fix
// delete the log entry await this.prisma.authorizationLog.delete({ where: { id: authorizationLog.id }, }); - console.log(dto); - return { tradeId: updatedTrade.id, success: true, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/src/modules/trades/trade.service.ts` at line 1355, Remove the leftover debug console.log by deleting the console.log(dto) call in confirmUnlockChainAction; locate the console.log in the confirmUnlockChainAction function in trade.service.ts and simply remove that statement so sensitive or unnecessary txHash/signature data are not output to logs.apps/frontend/components/bridge-ui/TradeAd.tsx (1)
179-201:⚠️ Potential issue | 🟡 Minor
handleCreateTradesilently swallowsmutateAsyncfailures and there is no error feedback.If
mutateAsyncrejects, execution skipssetSuccessSummary/toggleModal(good — the success modal won't lie), but the user gets no toast or other feedback in this component, and the rejection becomes an unhandled promise rejection from theonClickhandler. The siblingAddLiquidity.handleCreateAdwas just updated in this PR to wrap the call intry/catchand toast the error — apply the same pattern here for consistency.♻️ Suggested fix
const handleCreateTrade = async () => { if (!bridgerDstAddress) return if (!scaled || !scaled.ok) return - await mutateAsync({ - payload: { - adId: props.id, - routeId: props.routeId, - amount: scaled.orderAmount.toString(), - bridgerDstAddress, - }, - orderTokenId: props.orderTokenId, - }) - setSuccessSummary({ - payAmount: formatUnits(scaled.orderAmount, props.orderToken.decimals), - payTokenSymbol: props.orderToken.symbol, - receiveAmount: String(Number(amount) - txFee), - receiveTokenSymbol: props.adToken.symbol, - orderChainName, - adChainName, - recipient: bridgerDstAddress, - }) - toggleModal() + try { + await mutateAsync({ + payload: { + adId: props.id, + routeId: props.routeId, + amount: scaled.orderAmount.toString(), + bridgerDstAddress, + }, + orderTokenId: props.orderTokenId, + }) + setSuccessSummary({ + payAmount: formatUnits(scaled.orderAmount, props.orderToken.decimals), + payTokenSymbol: props.orderToken.symbol, + receiveAmount: String(Number(amount) - txFee), + receiveTokenSymbol: props.adToken.symbol, + orderChainName, + adChainName, + recipient: bridgerDstAddress, + }) + toggleModal() + } catch (err) { + // toast/error surface — match AddLiquidity.handleCreateAd + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/components/bridge-ui/TradeAd.tsx` around lines 179 - 201, Wrap the async call in handleCreateTrade in a try/catch so mutateAsync failures are caught and reported (like AddLiquidity.handleCreateAd): await mutateAsync(...) inside try, then call setSuccessSummary(...) and toggleModal() on success; in catch call the same toast/error helper used elsewhere (e.g., toast.error or the project’s useToast) with the caught error to surface the failure to the user and prevent an unhandled promise rejection; ensure you reference mutateAsync, setSuccessSummary, toggleModal, and handleCreateTrade when making the change.
🧹 Nitpick comments (11)
apps/backend-relayer/src/providers/noir/proof.service.ts (1)
19-19: Make thread count configurable instead of hardcoded.At Line 19,
threads: 4may over/under-utilize different runtime environments. Consider reading this from config/env (with a safe default) so ops can tune per deployment without code changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/src/providers/noir/proof.service.ts` at line 19, Replace the hardcoded threads: 4 with a configurable value read from environment or app config (e.g., process.env.NOIR_THREADS or the existing config service) inside the same module (proof.service.ts) where the threads option is set; parse the value to an integer, validate it (>0), and fall back to a safe default such as os.cpus().length or 4 if missing/invalid. Ensure you import/require os if using os.cpus(), use Number.parseInt and isNaN checks to validate, and apply the resulting numeric variable in place of the literal in the threads option so deployments can tune it via env/config.apps/frontend/app/globals.css (1)
159-164: Mirror the hover affordance for keyboard focus.The new underline/color treatment is hover-only. Add
:focus-visibleso keyboard users get the same navigation cue.♿ Proposed focus-visible parity
-.signal-line-link > a:hover { +.signal-line-link > a:hover, +.signal-line-link > a:focus-visible { color: var(--color-primary); } -.signal-line-link > a:hover::after { +.signal-line-link > a:hover::after, +.signal-line-link > a:focus-visible::after { transform: scaleX(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/app/globals.css` around lines 159 - 164, The hover-only underline/color affordance on .signal-line-link > a:hover and .signal-line-link > a:hover::after should also be applied for keyboard focus: add matching .signal-line-link > a:focus-visible and .signal-line-link > a:focus-visible::after rules that set color: var(--color-primary) and transform: scaleX(1) respectively (and ensure any transition/outline handling matches the hover behavior) so keyboard users receive the same visual cue.apps/frontend/app/(app)/bridge/page.tsx (1)
17-22: Remove the empty paragraph tag.The text update successfully generalizes the messaging. However, line 22 contains an empty
<p></p>tag that serves no purpose and should be removed.🧹 Proposed fix to remove dead code
<p className="text-[13px]"> Unlocking the next generation of cross-chain bridging{" "} <Link href={"/"} className="underline text-primary"> create ad </Link> </p> - <p></p> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/app/`(app)/bridge/page.tsx around lines 17 - 22, Remove the dead empty paragraph tag that follows the Link in the bridge page component: locate the block containing the text "Unlocking the next generation of cross-chain bridging" and the Link component (the JSX with Link href="/" and text "create ad") and delete the trailing empty <p></p> element so there is no blank paragraph rendered.scripts/relayer-e2e/flows/trade-lifecycle.ts (1)
291-301: Inline digest re-implementsstellarSignedMessageDigest— import it instead.The script already reaches into
apps/backend-relayer/.../utils/unlock-message.jsforbuildStellarUnlockMessage; doing the same for the digest helper keeps the signing preimage in lock-step with what the relayer actually verifies. Otherwise a future change to the prefix string or hash function insigning.tswill silently desynchronize the e2e flow from production.♻️ Proposed refactor
-import { createHash } from "crypto"; import { Keypair } from "@stellar/stellar-sdk"; import { getAddress, parseEther } from "viem"; import { privateKeyToAccount } from "viem/accounts"; import { TypedDataEncoder } from "ethers"; import { buildStellarUnlockMessage } from "../../../apps/backend-relayer/src/providers/stellar/utils/unlock-message.js"; +import { stellarSignedMessageDigest } from "../../../apps/backend-relayer/src/providers/stellar/utils/signing.js"; @@ - const bridgerMessage = buildStellarUnlockMessage({ - ...bridgerOrderParams, - orderHash: bridgerOrderHash, - }); - const bridgerDigest = createHash("sha256") - .update(Buffer.concat([ - Buffer.from("Stellar Signed Message:\n", "utf8"), - Buffer.from(bridgerMessage, "utf8"), - ])) - .digest(); - const bridgerSigBytes = bridgerStellar.sign(bridgerDigest); + const bridgerMessage = buildStellarUnlockMessage({ + ...bridgerOrderParams, + orderHash: bridgerOrderHash, + }); + const bridgerDigest = stellarSignedMessageDigest(bridgerMessage); + const bridgerSigBytes = bridgerStellar.sign(bridgerDigest);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/relayer-e2e/flows/trade-lifecycle.ts` around lines 291 - 301, The code re-implements the Stellar signed-message digest instead of using the shared helper; replace the manual creation of bridgerDigest with a call to the canonical helper stellarSignedMessageDigest(bridgerMessage) (import that function alongside buildStellarUnlockMessage) and pass its return value into bridgerStellar.sign so the e2e signing preimage matches production verification (update imports to include stellarSignedMessageDigest and remove the Buffer/sha256 manual logic around bridgerDigest).apps/backend-relayer/src/providers/stellar/utils/unlock-message.ts (1)
5-33: Duplicated message schema creates maintenance risk — shared utility would prevent future drift.Both
apps/backend-relayer/src/providers/stellar/utils/unlock-message.tsandapps/frontend/utils/stellar/unlock-message.tsimplement byte-identical JSON builders. Field order and types currently match perfectly, but because the implementations are separate, any future field addition/rename/reorder risks silencing unlock signature failures (verifyEd25519will fail with no obvious error trail).Instead of duplicating the builder, consider:
- Moving the builder to a shared types/utilities package both backend and frontend import from.
- If that's infeasible, add cross-reference comments in both files pointing to each other, plus a snapshot test that asserts both helpers emit identical JSON for a fixed
T_OrderParams/ITradeParamstest input.All
T_OrderParamsandITradeParamsfield types are JSON-safe (Bytes32Hex/Addressfrom viem are both0x${string}template types;amount, chain IDs, and salt are strings; decimals are numbers), so no bigint serialization risk exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/src/providers/stellar/utils/unlock-message.ts` around lines 5 - 33, The backend buildStellarUnlockMessage implementation duplicates the frontend builder (apps/frontend/utils/stellar/unlock-message.ts), risking drift; either extract the JSON builder into a shared utility/types package and have both import that single function (reference: buildStellarUnlockMessage) or, if sharing is infeasible, add explicit cross-reference comments in both files pointing to each other and add a snapshot test that imports both builders (backend's buildStellarUnlockMessage and the frontend's equivalent) and asserts their outputs are byte-identical for a fixed T_OrderParams/ITradeParams fixture (use a deterministic fixture including orderHash, salt, decimals, amounts, chain/token fields).apps/frontend/utils/stellar/unlock-message.ts (1)
5-29: Current implementations are synchronized; consider defensive measures to prevent future drift.The frontend and backend
buildStellarUnlockMessagehelpers currently stringify the identical set of fields in the same order, andITradeParamsincludes all required fields includingorderHash. No current schema mismatch exists.However, to guard against future divergence:
- Add a brief comment link between
apps/frontend/utils/stellar/unlock-message.tsandapps/backend-relayer/src/providers/stellar/utils/unlock-message.ts(e.g.,// MUST stay in sync with backend unlock-message.ts — any field/order difference breaks ed25519 verification).- Consider a fixture-based snapshot test that runs both implementations on shared test data and asserts byte-for-byte equality. This is especially valuable given the cryptographic sensitivity: even a single field reorder or type stringification difference would silently fail Stellar signature verification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/utils/stellar/unlock-message.ts` around lines 5 - 29, Add a defensive sync note and a cross-implementation snapshot test: put a succinct comment inside buildStellarUnlockMessage (e.g., "// MUST stay in sync with backend unlock-message.ts — any field/order difference breaks ed25519 verification") to tie it to apps/backend-relayer/src/providers/stellar/utils/unlock-message.ts, and add a fixture-based test that runs frontend buildStellarUnlockMessage and the backend equivalent on the same test data and asserts exact byte-for-byte equality of the JSON output to catch any future field/order/serialization drift.apps/frontend/app/(app)/(auth-protected)/orders/page.tsx (1)
29-42: MemoizelinkedAddressesfor consistency and stable query keys.
linkedAddressesis rebuilt on every render as a new array reference.apps/frontend/app/(app)/(auth-protected)/home/page.tsx(lines 21–24) wraps the equivalent computation inReact.useMemo. IfuseGetAllTradeskeys on reference identity (common with react-query serialization of params), the unstable reference could cause redundant refetches. Worth aligning both pages.♻️ Proposed refactor
const { data: currentUser } = useCurrentUser() - const linkedAddresses = - currentUser?.wallets?.map((w) => w.address).filter(Boolean) ?? [] + const linkedAddresses = useMemo( + () => currentUser?.wallets?.map((w) => w.address).filter(Boolean) ?? [], + [currentUser], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/app/`(app)/(auth-protected)/orders/page.tsx around lines 29 - 42, The linkedAddresses array is recreated on every render causing unstable references for useGetAllTrades' params; wrap the computation of linkedAddresses (derived from useCurrentUser()?.wallets) in React.useMemo so it returns a stable array identity across renders, then pass that memoized linkedAddresses into useGetAllTrades (the adCreatorAddress/bridgerAddress args) to prevent unnecessary refetches and ensure consistent query keys.apps/backend-relayer/src/modules/trades/dto/trade.dto.ts (1)
31-59: Consider per-element validation foradCreatorAddress/bridgerAddress.After removing
@IsString(), these fields accept any value that passes through@Transform. Non-string entries (e.g., numbers, booleans in an array) reach the service, which relies onnormalizeChainAddressthrowing inside atry/catch→BadRequestException('Invalid address filter'). This works but produces generic errors. Aclass-validator-level check gives better validation with field-specific messages.The split condition also differs from
participantAddresses: here, a comma-less string returns as-is (a plainstring), whileparticipantAddressesalways produces an array. That asymmetry is fine given thestring | string[]type.♻️ Proposed hardening
`@IsOptional`() `@Transform`(({ value }) => Array.isArray(value) ? value : typeof value === 'string' && value.includes(',') ? value.split(',').filter(Boolean) : value, ) + `@ValidateIf`((_, v) => v !== undefined) + `@IsString`({ each: true }) adCreatorAddress?: string | string[];(Apply the same to
bridgerAddress.@IsString({ each: true })validates arrays element-wise and also accepts a single string.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/src/modules/trades/dto/trade.dto.ts` around lines 31 - 59, Add per-element string validation to the adCreatorAddress and bridgerAddress DTO properties: keep the existing `@IsOptional`() and `@Transform`(...) logic but add `@IsString`({ each: true }) above each property so class-validator enforces that every array element (and single string) is a string; apply the same change to bridgerAddress and leave the current Transform behavior unchanged.apps/backend-relayer/src/providers/stellar/stellar.service.ts (1)
589-608: Protect against drift between frontend/backendbuildStellarUnlockMessage.Both implementations currently match perfectly in key order and indentation, producing identical JSON output. However, since these helpers are duplicated across
apps/backend-relayer/src/providers/stellar/utils/unlock-message.tsandapps/frontend/utils/stellar/unlock-message.ts, the risk of silent drift remains: adding a field or reordering keys in only one copy silently producesInvalid User Signatureat the adapter boundary with no obvious root cause.Two options, in order of preference:
- Share the helper from a single package (e.g.,
libs/shared) imported by bothapps/backend-relayerandapps/frontend, so there is only one source of truth.- If sharing isn't feasible, add a snapshot test on both sides pinning the exact serialized output for a fixed
T_OrderParamsfixture — any drift fails CI immediately.The current
sigBytes.length !== 64check correctly rejects signatures of any other length; consider adding an early string-shape check (e.g., reject input longer than ~200 chars) to avoid callingBuffer.from(..., 'base64')on clearly malformed input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/src/providers/stellar/stellar.service.ts` around lines 589 - 608, The verifyOrderSignature function risks silent drift because buildStellarUnlockMessage is duplicated; either move the unlock-message helper into a shared package (e.g., libs/shared) and import it into both apps so both backend verifyOrderSignature and the frontend use the single source of truth, or add snapshot tests in both apps that pin the exact serialized output of buildStellarUnlockMessage for a fixed T_OrderParams fixture so any divergence fails CI; additionally harden verifyOrderSignature by adding an early input-shape guard in verifyOrderSignature (reject overly long or clearly non-hex/base64 signature strings before calling Buffer.from) while keeping the existing sigBytes.length !== 64 check.apps/frontend/components/bridge-ui/CreateOrderSuccessModal.tsx (1)
91-96: “Submitted” timestamp is computed on every render rather than at submit time.
moment()is evaluated each render, so the displayed “Submitted” time drifts as React re-renders the modal. It's not the time the order was actually submitted — it's "now" at paint time. Consider includingsubmittedAt(astring/numbercaptured whensetSuccessSummaryis called) onCreatedOrderSummaryand rendering that, so the timestamp is stable and accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/components/bridge-ui/CreateOrderSuccessModal.tsx` around lines 91 - 96, The modal currently calls moment() during render in CreateOrderSuccessModal which yields a drifting "Submitted" time; change the data flow so the actual submit timestamp is captured when setSuccessSummary is called and stored on the CreatedOrderSummary object (add a submittedAt field of type string|number), then update CreateOrderSuccessModal to read and format summary.submittedAt (using moment(summary.submittedAt)) instead of calling moment() directly so the displayed timestamp is stable and reflects the real submit time.apps/frontend/utils/stellar/balance.ts (1)
78-81: WidenscValToNativecast for type accuracy — runtime is safe but type is too narrow.
scValToNativereturnsanyand can returnbigint(for i128/u128),number(for u32/i32),string(for some types),boolean,Uint8Array, arrays, maps, or the originalxdr.ScValdepending on the SCVal kind. Theas bigint | numbercast is narrower than the SDK's actual return type.BigInt(native)safely accepts all of these at runtime, but the cast masks type information and could silently break if the contract return type or SDK behavior changes. Widen toas bigint | number | string | booleanor use a runtime guard for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/utils/stellar/balance.ts` around lines 78 - 81, The narrow cast on the result of scValToNative risks losing type info; update the handling in the simulate path (where success.result.retval is converted) so that the value from scValToNative is widened (e.g. treat native as bigint | number | string | boolean) or remove the cast and add a small runtime guard before calling BigInt(native) that accepts bigint/number/string/boolean and converts appropriately; locate the conversion around the variable native (from scValToNative(success.result.retval)) and the BigInt(native) call (in this file) and implement the widened type or guard there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend-relayer/src/modules/trades/trade.service.ts`:
- Around line 901-926: Remove the leftover console.log debug statements from the
unlock flow: delete the console.log(e) inside the catch block that surrounds the
unlock/signature verification logic and delete the console.log(dto) that logs
the response object; locate these by finding the verifyOrderSignature call in
the unlock flow (references: orderParams,
this.chainAdapters.forChain(...).verifyOrderSignature, dto) and remove the two
console.log lines (optionally replace with existing processLogger methods if
structured logging is required).
In `@apps/frontend/app/`(app)/(auth-protected)/faucet/page.tsx:
- Around line 109-133: The Promise.race using freighterAddToken leaves the 30s
timer running and doesn't handle clipboard.writeText failures; fix by capturing
the timeout id returned by setTimeout before calling Promise.race (or wrap the
race so you can clearTimeout(timerId) when freighterAddToken settles) and call
clearTimeout(timerId) when the race resolves successfully or with the timeout
result, and when handling the res.error branch wrap
navigator.clipboard.writeText(contractId) in its own try/catch to catch
permission/rejection errors and surface a clear message (e.g., fallback UI
instruction to manually copy the contract id) instead of letting it bubble to
the outer catch; reference freighterAddToken, Promise.race, the timeout id, and
navigator.clipboard.writeText to locate the changes.
In `@apps/frontend/components/bridge-ui/TradeAd.tsx`:
- Around line 165-170: The insufficientBalance calculation treats
orderBalanceRaw === undefined as "not insufficient", enabling the Bridge button
while balances are still loading; update the logic in the insufficientBalance
computation (or the Bridge button disabled predicate) to treat unknown balance
as disabling—e.g. include the relevant loading flags so the result is
true/disabled when loading: reference scaled, orderBalanceRaw, and
isStellarOrder, and fold in stellarBalance.isLoading or (balance.isLoading ||
nativeBalance.isLoading) so the Bridge action cannot be clicked until the
balance queries complete.
In `@apps/frontend/components/orders/OrdersTable.tsx`:
- Around line 35-36: The ownsAddress helper lowercases addresses even though
backend guarantees canonical (normalized) addresses; update ownsAddress (and
callers comparing linkedAddresses with rowData.bridgerAddress/adCreatorAddress)
to either add a clarifying comment that addresses are expected pre-normalized by
the API or replace the lowercase call with a chain-aware normalization helper;
specifically, annotate the ownsAddress function with a brief comment about
backend normalization (or swap to a new normalizeAddressForComparison(addr,
chainKind?) helper) so the intent and fallback behavior are explicit and future
comparisons use the typed helper instead of ad-hoc toLowerCase().
In `@apps/frontend/components/shared/Header.tsx`:
- Around line 20-23: The external GitHub Link in Header (component Header.tsx,
JSX element Link with href "https://github.com/JoE11-y/ProofBridge" and
target="_blank") lacks rel="noopener noreferrer"; update that Link element to
include rel="noopener noreferrer" to prevent window.opener attacks and improve
security when opening in a new tab.
- Line 18: The parent div currently uses the "uppercase" utility in className
("flex items-center justify-between text-sm gap-8 uppercase") which cascades
into child components like ConnectWalletButton and alters wallet text; remove
"uppercase" from that parent className and instead add "uppercase" to the nav
link wrapper elements (the elements that wrap the navigation anchors) so only
the nav links are uppercased while ConnectWalletButton and other children retain
their original casing.
In `@apps/frontend/services/api.instance.ts`:
- Around line 64-77: In the refresh-failure branch of
apps/frontend/services/api.instance.ts, ensure any non-401 refresh errors also
clear auth cookies and propagate failure: when the api.post("/v1/auth/refresh",
...) inside the interceptor throws for any reason, remove/clear both
"auth_token" and "refresh_token" (same behavior as the isRefreshEndpoint branch)
before rethrowing so the auth-guard sees missing cookies. Also, when refresh
succeeds (you already set Cookies.set for res.data.tokens.access/refresh),
replay the original failed request using the saved error.config (e.g., set
Authorization header to the new access token on error.config.headers and return
await api.request(error.config)) so the caller receives the retried response
instead of the original rejection. Follow these changes around the existing
api.post call, error.config handling, and cookie set/remove logic.
---
Outside diff comments:
In `@apps/backend-relayer/src/modules/trades/trade.service.ts`:
- Line 1051: Stray console.log(e) in the catch block of TradeService should be
removed and replaced with the framework logger; locate the catch in
apps/backend-relayer/src/modules/trades/trade.service.ts (class TradeService /
the method containing the shown catch) and either drop the console.log(e)
entirely or call the project's logger at the appropriate level (e.g.
this.logger.error('Descriptive message', e) or Logger.error('Descriptive
message', e, 'TradeService')) so sensitive error objects are not printed to
stdout — ensure you use the existing logger instance/name used elsewhere in
TradeService for consistency.
- Line 1355: Remove the leftover debug console.log by deleting the
console.log(dto) call in confirmUnlockChainAction; locate the console.log in the
confirmUnlockChainAction function in trade.service.ts and simply remove that
statement so sensitive or unnecessary txHash/signature data are not output to
logs.
In `@apps/frontend/components/bridge-ui/TradeAd.tsx`:
- Around line 179-201: Wrap the async call in handleCreateTrade in a try/catch
so mutateAsync failures are caught and reported (like
AddLiquidity.handleCreateAd): await mutateAsync(...) inside try, then call
setSuccessSummary(...) and toggleModal() on success; in catch call the same
toast/error helper used elsewhere (e.g., toast.error or the project’s useToast)
with the caught error to surface the failure to the user and prevent an
unhandled promise rejection; ensure you reference mutateAsync,
setSuccessSummary, toggleModal, and handleCreateTrade when making the change.
In `@apps/frontend/utils/wagmi-config.ts`:
- Around line 2-20: The wagmi config removed hederaTestnet but other code
(chain-icons.ts and AdCard.tsx's explorer_urls) still expect it, causing
writeContractAsync calls to fail without a transport; either fully remove Hedera
references or re-add it to the wagmi configuration. Fix option A: remove
hederaTestnet usages by deleting Hedera entries from chain-icons.ts and
remove/guard explorer_urls in AdCard.tsx (references to explorer_urls and any
hedera chainId checks). Fix option B: re-add hederaTestnet into the chainList
and transports passed to createConfig (ensure you import the hederaTestnet
Chain, add it to chainList when appropriate, and provide a corresponding http
transport entry keyed by hederaTestnet.id) so writeContractAsync has a
transport; pick one approach and make the code changes consistently across
chain-icons.ts, AdCard.tsx (explorer_urls), and wagmi-config's createConfig.
---
Nitpick comments:
In `@apps/backend-relayer/src/modules/trades/dto/trade.dto.ts`:
- Around line 31-59: Add per-element string validation to the adCreatorAddress
and bridgerAddress DTO properties: keep the existing `@IsOptional`() and
`@Transform`(...) logic but add `@IsString`({ each: true }) above each property so
class-validator enforces that every array element (and single string) is a
string; apply the same change to bridgerAddress and leave the current Transform
behavior unchanged.
In `@apps/backend-relayer/src/providers/noir/proof.service.ts`:
- Line 19: Replace the hardcoded threads: 4 with a configurable value read from
environment or app config (e.g., process.env.NOIR_THREADS or the existing config
service) inside the same module (proof.service.ts) where the threads option is
set; parse the value to an integer, validate it (>0), and fall back to a safe
default such as os.cpus().length or 4 if missing/invalid. Ensure you
import/require os if using os.cpus(), use Number.parseInt and isNaN checks to
validate, and apply the resulting numeric variable in place of the literal in
the threads option so deployments can tune it via env/config.
In `@apps/backend-relayer/src/providers/stellar/stellar.service.ts`:
- Around line 589-608: The verifyOrderSignature function risks silent drift
because buildStellarUnlockMessage is duplicated; either move the unlock-message
helper into a shared package (e.g., libs/shared) and import it into both apps so
both backend verifyOrderSignature and the frontend use the single source of
truth, or add snapshot tests in both apps that pin the exact serialized output
of buildStellarUnlockMessage for a fixed T_OrderParams fixture so any divergence
fails CI; additionally harden verifyOrderSignature by adding an early
input-shape guard in verifyOrderSignature (reject overly long or clearly
non-hex/base64 signature strings before calling Buffer.from) while keeping the
existing sigBytes.length !== 64 check.
In `@apps/backend-relayer/src/providers/stellar/utils/unlock-message.ts`:
- Around line 5-33: The backend buildStellarUnlockMessage implementation
duplicates the frontend builder (apps/frontend/utils/stellar/unlock-message.ts),
risking drift; either extract the JSON builder into a shared utility/types
package and have both import that single function (reference:
buildStellarUnlockMessage) or, if sharing is infeasible, add explicit
cross-reference comments in both files pointing to each other and add a snapshot
test that imports both builders (backend's buildStellarUnlockMessage and the
frontend's equivalent) and asserts their outputs are byte-identical for a fixed
T_OrderParams/ITradeParams fixture (use a deterministic fixture including
orderHash, salt, decimals, amounts, chain/token fields).
In `@apps/frontend/app/`(app)/(auth-protected)/orders/page.tsx:
- Around line 29-42: The linkedAddresses array is recreated on every render
causing unstable references for useGetAllTrades' params; wrap the computation of
linkedAddresses (derived from useCurrentUser()?.wallets) in React.useMemo so it
returns a stable array identity across renders, then pass that memoized
linkedAddresses into useGetAllTrades (the adCreatorAddress/bridgerAddress args)
to prevent unnecessary refetches and ensure consistent query keys.
In `@apps/frontend/app/`(app)/bridge/page.tsx:
- Around line 17-22: Remove the dead empty paragraph tag that follows the Link
in the bridge page component: locate the block containing the text "Unlocking
the next generation of cross-chain bridging" and the Link component (the JSX
with Link href="/" and text "create ad") and delete the trailing empty <p></p>
element so there is no blank paragraph rendered.
In `@apps/frontend/app/globals.css`:
- Around line 159-164: The hover-only underline/color affordance on
.signal-line-link > a:hover and .signal-line-link > a:hover::after should also
be applied for keyboard focus: add matching .signal-line-link > a:focus-visible
and .signal-line-link > a:focus-visible::after rules that set color:
var(--color-primary) and transform: scaleX(1) respectively (and ensure any
transition/outline handling matches the hover behavior) so keyboard users
receive the same visual cue.
In `@apps/frontend/components/bridge-ui/CreateOrderSuccessModal.tsx`:
- Around line 91-96: The modal currently calls moment() during render in
CreateOrderSuccessModal which yields a drifting "Submitted" time; change the
data flow so the actual submit timestamp is captured when setSuccessSummary is
called and stored on the CreatedOrderSummary object (add a submittedAt field of
type string|number), then update CreateOrderSuccessModal to read and format
summary.submittedAt (using moment(summary.submittedAt)) instead of calling
moment() directly so the displayed timestamp is stable and reflects the real
submit time.
In `@apps/frontend/utils/stellar/balance.ts`:
- Around line 78-81: The narrow cast on the result of scValToNative risks losing
type info; update the handling in the simulate path (where success.result.retval
is converted) so that the value from scValToNative is widened (e.g. treat native
as bigint | number | string | boolean) or remove the cast and add a small
runtime guard before calling BigInt(native) that accepts
bigint/number/string/boolean and converts appropriately; locate the conversion
around the variable native (from scValToNative(success.result.retval)) and the
BigInt(native) call (in this file) and implement the widened type or guard
there.
In `@apps/frontend/utils/stellar/unlock-message.ts`:
- Around line 5-29: Add a defensive sync note and a cross-implementation
snapshot test: put a succinct comment inside buildStellarUnlockMessage (e.g.,
"// MUST stay in sync with backend unlock-message.ts — any field/order
difference breaks ed25519 verification") to tie it to
apps/backend-relayer/src/providers/stellar/utils/unlock-message.ts, and add a
fixture-based test that runs frontend buildStellarUnlockMessage and the backend
equivalent on the same test data and asserts exact byte-for-byte equality of the
JSON output to catch any future field/order/serialization drift.
In `@scripts/relayer-e2e/flows/trade-lifecycle.ts`:
- Around line 291-301: The code re-implements the Stellar signed-message digest
instead of using the shared helper; replace the manual creation of bridgerDigest
with a call to the canonical helper stellarSignedMessageDigest(bridgerMessage)
(import that function alongside buildStellarUnlockMessage) and pass its return
value into bridgerStellar.sign so the e2e signing preimage matches production
verification (update imports to include stellarSignedMessageDigest and remove
the Buffer/sha256 manual logic around bridgerDigest).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2311be28-f0a3-4a6f-9dda-6a559bf8e04b
⛔ Files ignored due to path filters (2)
apps/frontend/app/favicon.icois excluded by!**/*.icopnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
apps/backend-relayer/src/chain-adapters/adapters/chain-adapter.abstract.tsapps/backend-relayer/src/chain-adapters/adapters/evm-chain-adapter.tsapps/backend-relayer/src/chain-adapters/adapters/stellar-chain-adapter.tsapps/backend-relayer/src/modules/trades/dto/trade.dto.tsapps/backend-relayer/src/modules/trades/trade.service.tsapps/backend-relayer/src/providers/noir/proof.service.tsapps/backend-relayer/src/providers/stellar/stellar.service.tsapps/backend-relayer/src/providers/stellar/utils/signing.tsapps/backend-relayer/src/providers/stellar/utils/unlock-message.tsapps/backend-relayer/test/setups/mock-chain-adapter.tsapps/frontend/app/(app)/(auth-protected)/faucet/page.tsxapps/frontend/app/(app)/(auth-protected)/home/page.tsxapps/frontend/app/(app)/(auth-protected)/orders/page.tsxapps/frontend/app/(app)/bridge/page.tsxapps/frontend/app/globals.cssapps/frontend/components/ad-management-ui/AddLiquidity.tsxapps/frontend/components/bridge-ui/CreateOrderSuccessModal.tsxapps/frontend/components/bridge-ui/TradeAd.tsxapps/frontend/components/landing/Approach.tsxapps/frontend/components/landing/Features.tsxapps/frontend/components/landing/Hero-Alt.tsxapps/frontend/components/landing/Hero.tsxapps/frontend/components/landing/LandingHeader-Alt.tsxapps/frontend/components/landing/ReImagining.tsxapps/frontend/components/orders/OrdersTable.tsxapps/frontend/components/shared/Header.tsxapps/frontend/hooks/useTrades.tsapps/frontend/package.jsonapps/frontend/services/api.instance.tsapps/frontend/types/trades.tsapps/frontend/utils/stellar/balance.tsapps/frontend/utils/stellar/unlock-message.tsapps/frontend/utils/wagmi-config.tsscripts/relayer-e2e/flows/trade-lifecycle.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: trade lifecycle UI flow
Summary
End-to-end fixes for the trade unlock flow — Stellar signature verification, auth-token loops, and several UX dead-ends that made the lifecycle unusable without
manually signing out or guessing at missing wallet connections.
Details
Stellar unlock signature verification
through uuidToBigInt, producing a garbage hash that never matched the frontend's).
fields instead of a bare 32-byte hex.
Unlock-confirm 404
relayer's on-chain request signature — two different strings. The filter was dropped; tradeId + userAddress IN callerLinked is sufficient, and reqHash (validated
on-chain inside confirm) remains the true integrity gate.
Auth token handling (api.instance.ts)
the missing cookie.
UX fixes
errors instead of swallowing them. Previously an ETH → Stellar ad with no Stellar wallet linked would silently throw inside the confirm modal.
getWalletClient after switchChainAsync so watchAsset doesn't silently no-op on a stale client.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation & UX Updates