Race Condition in approve + write (Transaction Failure)#70
Open
aniket866 wants to merge 3 commits into
Open
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughThis PR migrates auction service write operations from synchronous to asynchronous contract calls by switching from Changes
Sequence DiagramsequenceDiagram
participant Component as UI Component
participant Service as Auction Service
participant WriteAsync as writeContractAsync
participant PublicClient as publicClient
participant Blockchain as Blockchain
Component->>Service: createAuction(writeContractAsync, publicClient, params)
Service->>Service: approveToken(writeContractAsync)
Service->>WriteAsync: Execute approve transaction
WriteAsync->>Blockchain: Submit approve tx
Blockchain-->>WriteAsync: Return approvalHash
Service->>PublicClient: waitForTransactionReceipt(approvalHash)
PublicClient->>Blockchain: Poll for receipt
Blockchain-->>PublicClient: Approval confirmed
Service->>WriteAsync: Execute createAuction transaction
WriteAsync->>Blockchain: Submit auction tx
Blockchain-->>WriteAsync: Return tx hash
Service-->>Component: Promise resolved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
Contributor
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 (7)
components/auction/bid-form.tsx (1)
188-189:⚠️ Potential issue | 🟠 MajorMissing null-checks for
writeContractAsyncandpublicClientin submit handler.The guard at line 189 checks
isConnectedandaddress, but doesn't validatewriteContractAsyncorpublicClient. Both can beundefinedand are used without null-checks at lines 204-205 and 215-216, which will cause runtime errors.Proposed fix
const handleSubmit = async () => { - if (!isValidBid || !isConnected || !address || isSubmitting) return; + if (!isValidBid || !isConnected || !address || isSubmitting || !writeContractAsync || !publicClient) return;Also applies to: 203-208, 214-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auction/bid-form.tsx` around lines 188 - 189, The submit handler handleSubmit currently guards on isValidBid, isConnected, address, and isSubmitting but uses writeContractAsync and publicClient later; update handleSubmit to also check that writeContractAsync and publicClient are defined (e.g., if (!writeContractAsync || !publicClient) return or set an error) before proceeding with the calls that rely on them (the async writeContractAsync(...) invocation and publicClient.waitForTransaction / getTransaction), and ensure any code paths that assume these objects are present are guarded to avoid runtime undefined errors.lib/services/logarithmic-dutch-auction-service.ts (1)
190-213:⚠️ Potential issue | 🔴 CriticalBug:
waitForTransactionReceiptcalled with undefined hash whencurrentPriceis zero.When
currentPrice === BigInt(0), the approval is skipped (lines 193-202), butapprovalHashwill be undefined. Line 201 then attempts to wait for an undefined hash, causing a runtime error. ThewaitForTransactionReceiptcall should be inside the conditional block.Proposed fix
async placeBid(writeContract: WriteContractMutateAsync<Config, unknown>, publicClient: UsePublicClientReturnType, auctionId: bigint, biddingToken: string): Promise<void> { try { const currentPrice = await this.getCurrentPrice(auctionId); if (currentPrice !== BigInt(0)) { const approvalHash = await this.approveToken( writeContract, biddingToken as `0x${string}`, this.contractAddress, currentPrice, false ); - } - await publicClient!.waitForTransactionReceipt({ hash: approvalHash }); + await publicClient!.waitForTransactionReceipt({ hash: approvalHash }); + } await writeContract({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/logarithmic-dutch-auction-service.ts` around lines 190 - 213, In placeBid, the code calls publicClient.waitForTransactionReceipt with approvalHash even when currentPrice === BigInt(0) (approval is skipped), causing a runtime error; fix by moving the await publicClient!.waitForTransactionReceipt({ hash: approvalHash }) inside the if (currentPrice !== BigInt(0)) block (or guard it with if (approvalHash) after approveToken) so it's only invoked when approveToken actually returned a hash (reference: function placeBid, variables currentPrice and approvalHash, and publicClient.waitForTransactionReceipt).components/auction/dutch-auction-price.tsx (1)
1-1:⚠️ Potential issue | 🟠 MajorMissing
"use client"directive.This component uses React hooks (
useState,useEffect) and wagmi hooks (useWriteContract,useChainId,usePublicClient) which require client-side rendering. Add the directive at the top of the file.Proposed fix
+"use client"; + import { useEffect, useState } from "react";As per coding guidelines:
**/*.{ts,tsx,js,jsx}: NextJS: Ensure that "use client" is being used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/auction/dutch-auction-price.tsx` at line 1, This file is missing the required "use client" directive for components using React and wagmi hooks; add a top-of-file "use client" directive as the very first line above the import, so the DutchAuctionPrice component can safely use useState, useEffect, and wagmi hooks like useWriteContract, useChainId, and usePublicClient.lib/services/allpay-auction-service.ts (2)
86-118:⚠️ Potential issue | 🟠 MajorGuard against undefined
publicClientbefore callingwaitForTransactionReceipt.
UsePublicClientReturnTypecan beundefinedwhen the wallet is not connected. The non-null assertion (!) on line 95 will cause a runtime error in that case.🛡️ Proposed fix to add null check
const approvalHash = await this.approveToken( writeContract, params.auctionedToken, this.contractAddress, (params.auctionType === BigInt(0) ? params.auctionedTokenIdOrAmount : parseEther(String(params.auctionedTokenIdOrAmount))), params.auctionType === BigInt(0) ); - await publicClient!.waitForTransactionReceipt({ hash: approvalHash }); + if (!publicClient) { + throw new Error("Public client is not available"); + } + await publicClient.waitForTransactionReceipt({ hash: approvalHash });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/allpay-auction-service.ts` around lines 86 - 118, The createAuction method uses publicClient!.waitForTransactionReceipt with a non-null assertion which can throw if publicClient is undefined; update createAuction to guard against undefined publicClient (e.g., check if publicClient is truthy before calling waitForTransactionReceipt), handle the no-client case by either awaiting an alternative flow or throwing a clear error, and reference the symbol publicClient and the method waitForTransactionReceipt so the null-check is added before the await and any subsequent logic that assumes a connected wallet.
120-146:⚠️ Potential issue | 🟠 MajorSame non-null assertion risk on
publicClientinplaceBid.Line 135 uses
publicClient!.waitForTransactionReceiptwhich will throw at runtime ifpublicClientisundefined.🛡️ Proposed fix
const approvalHash = await this.approveToken( writeContract, biddingTokenAddress, this.contractAddress, bidAmount, false // 0 = NFT, 1 = ERC20 ); - await publicClient!.waitForTransactionReceipt({ hash: approvalHash }); + if (!publicClient) { + throw new Error("Public client is not available"); + } + await publicClient.waitForTransactionReceipt({ hash: approvalHash });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/allpay-auction-service.ts` around lines 120 - 146, The placeBid method uses publicClient!.waitForTransactionReceipt which can throw if publicClient is undefined; update placeBid to explicitly guard that publicClient is present (e.g., if (!publicClient) throw a clear error or return) before calling waitForTransactionReceipt, or use safe access (optional chaining) and handle the undefined case; reference the placeBid method and the publicClient.waitForTransactionReceipt call to locate and fix the guard so you never use the non-null assertion on publicClient at runtime.lib/services/vickrey-auction-service.ts (2)
143-175:⚠️ Potential issue | 🟠 MajorGuard against undefined
publicClientbefore callingwaitForTransactionReceipt.
UsePublicClientReturnTypecan beundefinedwhen the wallet is not connected. Using the non-null assertion (!) on line 152 will cause a runtime error in that case.🛡️ Proposed fix to add null check
const approvalHash = await this.approveToken( writeContract, params.auctionedToken, this.contractAddress, (params.auctionType === BigInt(0) ? params.auctionedTokenIdOrAmount : parseEther(String(params.auctionedTokenIdOrAmount))), params.auctionType === BigInt(0) // 0 = NFT, 1 = ERC20 ); - await publicClient!.waitForTransactionReceipt({ hash: approvalHash }); + if (!publicClient) { + throw new Error("Public client is not available"); + } + await publicClient.waitForTransactionReceipt({ hash: approvalHash });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/vickrey-auction-service.ts` around lines 143 - 175, In createAuction, guard against publicClient being undefined before calling publicClient.waitForTransactionReceipt: check the publicClient (UsePublicClientReturnType) early in the method (before awaiting approvalHash or before calling waitForTransactionReceipt) and handle the undefined case by either throwing a clear error or returning/deferring until the wallet is connected; remove the non-null assertion (!), use the publicClient instance only after the null-check, and ensure the error path is surfaced from createAuction so callers can handle the missing public client.
192-214:⚠️ Potential issue | 🟠 MajorSame non-null assertion risk on
publicClientinrevealBid.Line 203 uses
publicClient!.waitForTransactionReceiptwhich will throw at runtime ifpublicClientisundefined.🛡️ Proposed fix
const approvalHash = await this.approveToken( writeContract, biddingToken as `0x${string}`, this.contractAddress, bidAmount, false ); - await publicClient!.waitForTransactionReceipt({ hash: approvalHash }); + if (!publicClient) { + throw new Error("Public client is not available"); + } + await publicClient.waitForTransactionReceipt({ hash: approvalHash });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/vickrey-auction-service.ts` around lines 192 - 214, The revealBid method uses a non-null assertion on publicClient when calling publicClient!.waitForTransactionReceipt which can throw if publicClient is undefined; update revealBid to validate that publicClient is present before using it (e.g., throw a clear error or return early if publicClient is falsy) and then call waitForTransactionReceipt on the validated object, or change the method signature to require a non-null publicClient; reference the revealBid method and the waitForTransactionReceipt call when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/create/page.tsx`:
- Around line 309-310: The handleSubmit function currently guards only address
and writeContractAsync but not publicClient (from usePublicClient()); update the
guard at the start of handleSubmit (and the similar spot around line 345) to
also check that publicClient is defined before calling
auctionService.createAuction; if publicClient is undefined, return early (or
surface an error) so auctionService.createAuction and its internal
waitForTransactionReceipt are never invoked with an undefined client.
In `@components/auction/dutch-auction-price.tsx`:
- Around line 70-78: handleBuyout calls getAuctionService(...).getAuction(...)
and then passes client into placeBid without ensuring client is defined; add a
null-check for the result of usePublicClient (client) at the start of
handleBuyout (same guard area as writeContractAsync/isEnded) and early-return or
surface an error if client is undefined so you never call
dutchService.getAuction or dutchService.placeBid with an undefined client.
Update the control flow in handleBuyout (and any subsequent calls to
getAuctionService, auction.getAuction, and placeBid) to only proceed when client
is non-null to prevent runtime errors like waitForTransactionReceipt receiving
undefined.
In `@components/auction/vickrey-reveal-form.tsx`:
- Line 36: The code calls usePublicClient() and may pass its result into
vickreyService.revealBid without checking for undefined; add a guard like the
existing writeContractAsync check to bail out and surface an error when
publicClient is falsy. Locate the usePublicClient() usage and ensure you check
for publicClient before calling vickreyService.revealBid (and any other methods
that call waitForTransactionReceipt), returning or throwing an appropriate
error/logging message if publicClient is undefined; apply the same null-check
pattern around the other reveal call sites referenced (the block around the
writeContractAsync pattern and the other occurrence near the second reveal
call).
In `@lib/services/english-auction-service.ts`:
- Line 148: The code uses a non-null assertion on publicClient when calling
waitForTransactionReceipt (publicClient!.waitForTransactionReceipt), which can
crash if publicClient is undefined; update the service to either (a) perform an
early guard like const client = this.publicClient; if (!client) throw new
Error('publicClient not initialized'); then call
client.waitForTransactionReceipt({ hash: approvalHash }) (apply the same guard
to the other occurrence), or (b) change the service interface so publicClient is
non-nullable and ensure callers always pass a valid client; replace all
publicClient! usages (e.g., the waitForTransactionReceipt calls) with the
guarded client variable to eliminate the non-null assertion.
In `@lib/services/exponential-dutch-auction-service.ts`:
- Line 134: The code uses a non-null assertion on publicClient when calling
publicClient!.waitForTransactionReceipt which can crash if publicClient is
undefined; update the surrounding logic (e.g., in the functions that call
waitForTransactionReceipt) to validate publicClient is present before calling it
(throw or return a clear error if missing), or require the caller to pass a
validated client; replace the `publicClient!` usage with a safe access pattern
(guard clause checking `if (!publicClient) { throw new Error("publicClient not
available"); }`) before invoking waitForTransactionReceipt (also apply the same
guard at the other occurrence that calls
publicClient.waitForTransactionReceipt).
In `@lib/services/linear-dutch-auction-service.ts`:
- Line 131: The code uses a non-null assertion on publicClient
(publicClient!.waitForTransactionReceipt) which can crash at runtime; either
validate publicClient at the start of the method in LinearDutchAuctionService
(throw or return an error if undefined) and remove the "!" before calling
waitForTransactionReceipt, or change the method's signature to require a
non-null PublicClient so callers must provide it; apply the same fix for the
other occurrence around the waitForTransactionReceipt call at the other location
(line ~165).
In `@lib/services/logarithmic-dutch-auction-service.ts`:
- Line 165: The code uses a non-null assertion on publicClient (e.g., the call
to publicClient!.waitForTransactionReceipt) which can crash at runtime; update
the surrounding method(s) in logarithmic-dutch-auction-service to either (A)
validate publicClient at the start (if undefined, throw a clear error or return
a rejected Promise) before any calls like waitForTransactionReceipt, or (B)
change the method signature so publicClient is non-nullable and enforce that at
callers; replace all occurrences of publicClient! (including the other reported
usage) with safe, explicit checks and early exits so runtime crashes are
prevented.
---
Outside diff comments:
In `@components/auction/bid-form.tsx`:
- Around line 188-189: The submit handler handleSubmit currently guards on
isValidBid, isConnected, address, and isSubmitting but uses writeContractAsync
and publicClient later; update handleSubmit to also check that
writeContractAsync and publicClient are defined (e.g., if (!writeContractAsync
|| !publicClient) return or set an error) before proceeding with the calls that
rely on them (the async writeContractAsync(...) invocation and
publicClient.waitForTransaction / getTransaction), and ensure any code paths
that assume these objects are present are guarded to avoid runtime undefined
errors.
In `@components/auction/dutch-auction-price.tsx`:
- Line 1: This file is missing the required "use client" directive for
components using React and wagmi hooks; add a top-of-file "use client" directive
as the very first line above the import, so the DutchAuctionPrice component can
safely use useState, useEffect, and wagmi hooks like useWriteContract,
useChainId, and usePublicClient.
In `@lib/services/allpay-auction-service.ts`:
- Around line 86-118: The createAuction method uses
publicClient!.waitForTransactionReceipt with a non-null assertion which can
throw if publicClient is undefined; update createAuction to guard against
undefined publicClient (e.g., check if publicClient is truthy before calling
waitForTransactionReceipt), handle the no-client case by either awaiting an
alternative flow or throwing a clear error, and reference the symbol
publicClient and the method waitForTransactionReceipt so the null-check is added
before the await and any subsequent logic that assumes a connected wallet.
- Around line 120-146: The placeBid method uses
publicClient!.waitForTransactionReceipt which can throw if publicClient is
undefined; update placeBid to explicitly guard that publicClient is present
(e.g., if (!publicClient) throw a clear error or return) before calling
waitForTransactionReceipt, or use safe access (optional chaining) and handle the
undefined case; reference the placeBid method and the
publicClient.waitForTransactionReceipt call to locate and fix the guard so you
never use the non-null assertion on publicClient at runtime.
In `@lib/services/logarithmic-dutch-auction-service.ts`:
- Around line 190-213: In placeBid, the code calls
publicClient.waitForTransactionReceipt with approvalHash even when currentPrice
=== BigInt(0) (approval is skipped), causing a runtime error; fix by moving the
await publicClient!.waitForTransactionReceipt({ hash: approvalHash }) inside the
if (currentPrice !== BigInt(0)) block (or guard it with if (approvalHash) after
approveToken) so it's only invoked when approveToken actually returned a hash
(reference: function placeBid, variables currentPrice and approvalHash, and
publicClient.waitForTransactionReceipt).
In `@lib/services/vickrey-auction-service.ts`:
- Around line 143-175: In createAuction, guard against publicClient being
undefined before calling publicClient.waitForTransactionReceipt: check the
publicClient (UsePublicClientReturnType) early in the method (before awaiting
approvalHash or before calling waitForTransactionReceipt) and handle the
undefined case by either throwing a clear error or returning/deferring until the
wallet is connected; remove the non-null assertion (!), use the publicClient
instance only after the null-check, and ensure the error path is surfaced from
createAuction so callers can handle the missing public client.
- Around line 192-214: The revealBid method uses a non-null assertion on
publicClient when calling publicClient!.waitForTransactionReceipt which can
throw if publicClient is undefined; update revealBid to validate that
publicClient is present before using it (e.g., throw a clear error or return
early if publicClient is falsy) and then call waitForTransactionReceipt on the
validated object, or change the method signature to require a non-null
publicClient; reference the revealBid method and the waitForTransactionReceipt
call when making the change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37b2118a-00ec-4e50-8116-382089356e0b
📒 Files selected for processing (11)
app/create/page.tsxcomponents/auction/bid-form.tsxcomponents/auction/dutch-auction-price.tsxcomponents/auction/vickrey-reveal-form.tsxlib/auction-service.tslib/services/allpay-auction-service.tslib/services/english-auction-service.tslib/services/exponential-dutch-auction-service.tslib/services/linear-dutch-auction-service.tslib/services/logarithmic-dutch-auction-service.tslib/services/vickrey-auction-service.ts
Contributor
Author
|
@coderabbitai review and approve |
Contributor
🧩 Analysis chain🏁 Script executed: Length of output: 104749
However, I found several issues that should be addressed before approval: 🔴 Issue 1 — Null-check on
|
Contributor
Author
|
@coderabbitai review and approve |
Contributor
|
Kicking off the review now to check the current state of the code. ✅ Actions performedFull review triggered. |
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.
Addressed Issues:
Closes #32
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit