Implement fully automated Merkle Proof generation for Claims #163
Conversation
- Added MerkleTreeUtils for local proof calculation from shards - Added GetAllowlistProof extension method to fetch and generate proofs - Updated DropERC20, DropERC721, and DropERC1155 Claim methods to use dynamic proofs - Added necessary data types in ThirdwebExtensions.Types.cs - Fixed global.json to support available .NET 8 SDK - Optimized string comparisons for performance
WalkthroughAdds Merkle-based allowlist support: new metadata types and proof structures, MerkleTreeUtils for proof calculation, a GetAllowlistProof extension used in claim flows, a MAX_UINT256_STR constant, and global.json SDK update. (47 words) Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Contract as ThirdwebContract
participant Extension as GetAllowlistProof
participant Metadata as Contract Metadata (URI)
participant ShardStore as Shard HTTP Store
participant MerkleUtil as MerkleTreeUtils
participant ClaimOp as Claim Operation
Caller->>Extension: GetAllowlistProof(wallet, claimConditionId?, tokenId?)
activate Extension
Extension->>Contract: Fetch metadata (merkle info)
Metadata-->>Extension: Merkle tree info
alt Merkle info present & non-zero root
Extension->>ShardStore: Resolve shard URI (GetShardKey) & GET shard JSON
ShardStore-->>Extension: ShardData (entries, proofs)
Extension->>MerkleUtil: CalculateMerkleProof(ShardData, wallet)
MerkleUtil->>MerkleUtil: Hash leaves, build tree, locate claimer, assemble proof
MerkleUtil-->>Extension: AllowlistProof (proof bytes, limits, price, currency)
Extension-->>Caller: AllowlistProof
else No merkle / public mint / fetch error
Extension-->>Caller: null or empty AllowlistProof
end
deactivate Extension
Caller->>ClaimOp: Claim(tx params + AllowlistProof)
activate ClaimOp
ClaimOp->>ClaimOp: Validate/encode proof, compute payableAmount, submit tx
ClaimOp-->>Caller: tx result
deactivate ClaimOp
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 1
🧹 Nitpick comments (2)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (1)
1587-1704: Consider more granular error handling.The entire
GetAllowlistProofmethod is wrapped in a try-catch that returnsnullfor any exception. While this prevents crashes, it may hide legitimate errors (network issues, contract call failures, etc.) that developers should be aware of.Consider logging exceptions or returning more specific error information, especially for debugging:
catch { + // Consider logging the exception for debugging + // or rethrowing specific exception types return null; }Thirdweb/Thirdweb.Utils/MerkleTree.cs (1)
111-149: LGTM!The Merkle tree construction correctly implements OpenZeppelin-compatible tree building:
- Sorts leaves lexicographically
- Builds layers bottom-up
- Handles odd nodes by promoting to next layer
- Properly pairs and hashes adjacent nodes
Note: The
leafToEntryMapdictionary (line 113) is created but not used. This is harmless but could be removed for code clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.Types.cs(1 hunks)Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs(4 hunks)Thirdweb/Thirdweb.Utils/MerkleTree.cs(1 hunks)global.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Thirdweb/Thirdweb.Utils/MerkleTree.cs (3)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.Types.cs (2)
WhitelistEntry(149-174)ShardData(131-144)Thirdweb/Thirdweb.Utils/Constants.cs (1)
Constants(3-55)Thirdweb/Thirdweb.Utils/Utils.cs (2)
BytesToHex(119-122)HexToBytes(129-132)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (2)
Thirdweb/Thirdweb.Utils/MerkleTree.cs (2)
AllowlistProof(95-220)GetShardKey(228-237)Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.Types.cs (3)
ContractMetadata(54-86)MerkleTreeInfo(95-126)ShardData(131-144)
🔇 Additional comments (15)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.Types.cs (4)
79-86: LGTM!The
Merkleproperty addition is well-documented and uses appropriate types for storing merkle root to IPFS URI mappings.
88-126: LGTM!The
MerkleTreeInfoclass is well-structured with appropriate default values forShardNybbles(2) andTokenDecimals(0).
128-174: LGTM!The
ShardDataandWhitelistEntryclasses are well-designed. Usingstringtypes forMaxClaimableandPriceprovides flexibility to handle both "unlimited" values and numeric values, which is properly documented.
176-205: LGTM!The
AllowlistProofclass has correct ABI parameter definitions matching the expected smart contract interface for Merkle proof verification.Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (5)
1571-1585: LGTM!Method signature and parameter validation are well-defined with appropriate exceptions for null/empty inputs.
1647-1699: LGTM!The merkle root lookup includes appropriate case-insensitive fallback, and the shard retrieval handles both
.jsonextension and no extension gracefully. The use ofStringComparison.OrdinalIgnoreCaseis a good performance optimization.
1755-1780: LGTM!The allowlist proof integration in
DropERC20_Claimcorrectly handles:
- Fetching proof data
- Building the proof payload
- Recalculating payable amount when price overrides exist
- Checking both native token address variants
The same pattern is consistently applied across all claim methods.
1917-1942: LGTM!The allowlist proof integration in
DropERC721_Claimfollows the same solid pattern as DropERC20_Claim.
2104-2129: LGTM!The allowlist proof integration in
DropERC1155_Claimcorrectly passes thetokenIdparameter toGetAllowlistProof, which is necessary for ERC1155 contracts with per-token claim conditions.Thirdweb/Thirdweb.Utils/MerkleTree.cs (5)
20-50: LGTM!The
HashLeafimplementation correctly:
- Normalizes addresses to lowercase
- Uses
StringComparison.OrdinalIgnoreCasefor efficient "unlimited" checks (as mentioned in PR)- Handles missing/unlimited values appropriately with MAX_UINT256 and ADDRESS_ZERO defaults
- Uses ABI packed encoding and Keccak256 as per OpenZeppelin standards
55-87: LGTM!The
CompareBytesandHashPairmethods correctly implement OpenZeppelin-compatible Merkle tree operations:
- Lexicographic byte comparison
- Sorted pair hashing (smaller first)
- Efficient byte array operations with
Buffer.BlockCopy
95-109: LGTM!The validation and claimer lookup logic correctly:
- Validates shard data before processing
- Normalizes addresses for consistent comparison
- Uses case-insensitive address matching
151-220: LGTM!The Merkle proof generation is correctly implemented:
- Locates the claimer's leaf in the sorted array
- Builds the proof path by collecting sibling nodes at each layer
- Correctly determines left/right positioning (index % 2)
- Combines local mini-proof with shard-level proofs
- Parses entry values consistently with
HashLeaflogic
228-237: LGTM!The
GetShardKeymethod correctly:
- Normalizes the address to lowercase
- Strips the "0x" prefix
- Returns the specified number of hex characters
- Uses
Math.Minto prevent out-of-bounds accessglobal.json (1)
3-4: global.json is newly introduced, not a downgrade.This is a new global.json file being created for the first time in the repository, not a modification of an existing configuration. There is no evidence of any prior SDK version specified (8.0.401 or otherwise). The commit message explicitly states the purpose is to "support available .NET 8 SDK," and the configuration using 8.0.100 with
rollForward: latestFeatureis appropriate—it pins a known stable version while allowing newer SDK versions to be used when available.Likely an incorrect or invalid review comment.
|
Thank you for this! Will review next week and release if all is well. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (2)
1634-1635: Consider extracting the zero merkle root to a constant.The hardcoded value
"0x0000000000000000000000000000000000000000000000000000000000000000"is used to identify public mints. Extracting it to a named constant (e.g.,ZERO_MERKLE_ROOT) in theConstantsclass would improve maintainability and make the intent clearer.Apply this diff:
+public const string ZERO_MERKLE_ROOT = "0x0000000000000000000000000000000000000000000000000000000000000000";Then update line 1634:
- if (merkleRootHex == "0x0000000000000000000000000000000000000000000000000000000000000000") + if (merkleRootHex == Constants.ZERO_MERKLE_ROOT)
1756-1781: Consider extracting duplicate payable amount calculation logic.The logic for recalculating
payableAmountwhen an allowlist has a price override (lines 1769-1776) is duplicated inDropERC721_Claim(lines 1931-1938) andDropERC1155_Claim(lines 2118-2125). Extracting this to a helper method would improve maintainability.Example helper:
private static BigInteger CalculatePayableAmount( AllowlistProof proofData, BigInteger defaultAmount, BigInteger quantity) { var maxUint256 = BigInteger.Parse(Constants.MAX_UINT256_STR); if (proofData.PricePerToken < maxUint256) { var currency = proofData.Currency; var isNativeToken = currency == Constants.NATIVE_TOKEN_ADDRESS || currency == Constants.ADDRESS_ZERO; return isNativeToken ? quantity * proofData.PricePerToken : BigInteger.Zero; } return defaultAmount; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs(4 hunks)Thirdweb/Thirdweb.Utils/Constants.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (4)
Thirdweb/Thirdweb.Utils/MerkleTree.cs (3)
AllowlistProof(95-220)MerkleTreeUtils(10-238)GetShardKey(228-237)Thirdweb.Tests/Thirdweb.Contracts/Thirdweb.Contracts.Tests.cs (1)
AllowlistProof(133-139)Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.Types.cs (3)
ContractMetadata(54-86)MerkleTreeInfo(95-126)ShardData(131-144)Thirdweb/Thirdweb.Utils/Constants.cs (1)
Constants(3-56)
🔇 Additional comments (5)
Thirdweb/Thirdweb.Utils/Constants.cs (1)
16-16: LGTM! Addresses previous feedback.The addition of
MAX_UINT256_STRsuccessfully resolves the duplicate hardcoded value issue flagged in previous reviews. The constant is now properly defined and used throughout the Merkle/allowlist logic.Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (4)
1560-1586: LGTM! Well-structured method signature and validation.The method signature is clear with appropriate optional parameters, and input validation follows defensive programming best practices.
1650-1660: Good defensive programming with case-insensitive fallback.The case-insensitive merkle root lookup (line 1654) handles potential inconsistencies in metadata formatting, improving robustness.
1918-1943: LGTM! Consistent allowlist integration.The allowlist proof integration correctly handles both allowlist and public mint scenarios, with proper null checks and fallback to default empty proofs.
2105-2130: LGTM! Correct tokenId parameter for ERC1155.The integration correctly passes the
tokenIdparameter toGetAllowlistProof(line 2106), which is necessary for ERC1155 drops that may have different claim conditions per token.
| catch (Exception) | ||
| { | ||
| // TODO: Log exception for debugging without crashing | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ThirdwebExtensions.cs file
find . -name "ThirdwebExtensions.cs" -type f 2>/dev/nullRepository: thirdweb-dev/dotnet
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Read lines 1698-1710 to see the full context of the exception handling
sed -n '1698,1710p' ./Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.csRepository: thirdweb-dev/dotnet
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Get broader context - lines 1680-1710 to see the method signature and full try-catch
sed -n '1680,1710p' ./Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.csRepository: thirdweb-dev/dotnet
Length of output: 1006
🏁 Script executed:
#!/bin/bash
# Get the full method including signature - lines 1660-1710
sed -n '1660,1710p' ./Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.csRepository: thirdweb-dev/dotnet
Length of output: 1684
🏁 Script executed:
#!/bin/bash
# Get the method signature - lines 1630-1665
sed -n '1630,1665p' ./Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.csRepository: thirdweb-dev/dotnet
Length of output: 1464
🏁 Script executed:
#!/bin/bash
# Get the method signature and start - lines 1600-1635
sed -n '1600,1635p' ./Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.csRepository: thirdweb-dev/dotnet
Length of output: 1937
🏁 Script executed:
#!/bin/bash
# Get the method signature - lines 1550-1605
sed -n '1550,1605p' ./Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.csRepository: thirdweb-dev/dotnet
Length of output: 2448
🏁 Script executed:
#!/bin/bash
# Check if there's a logging infrastructure in the codebase
# Look for common logging patterns
rg -l "ILogger|logger|log|Debug|Trace|Error" --type cs | head -20Repository: thirdweb-dev/dotnet
Length of output: 1172
🏁 Script executed:
#!/bin/bash
# Check what logging patterns are used in ThirdwebExtensions.cs
rg "Debug|log|Log|error|Error" ./Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs | head -20Repository: thirdweb-dev/dotnet
Length of output: 1518
🏁 Script executed:
#!/bin/bash
# Check how logging is actually done in the codebase - look for actual log calls
rg "System.Diagnostics.Debug|Console.WriteLine|logger\." --type cs -A 2 | head -40Repository: thirdweb-dev/dotnet
Length of output: 2342
🏁 Script executed:
#!/bin/bash
# Check ThirdwebContract and ThirdwebClient for logging setup
rg "Log|log|Logger|ILogger" ./Thirdweb/Thirdweb.Contracts/ThirdwebContract.cs 2>/dev/null | head -10
rg "Log|log|Logger|ILogger" ./Thirdweb/Thirdweb.Client/ThirdwebClient.cs 2>/dev/null | head -10Repository: thirdweb-dev/dotnet
Length of output: 45
Broad exception handling lacks production observability.
The catch-all exception handler silently returns null without logging, making it impossible to debug failures in production. While the method documents returning null for expected cases (missing allowlist or public mint), unexpected errors during Merkle tree calculation or shard download are also silently swallowed.
Note: The suggested System.Diagnostics.Debug.WriteLine() only works in Debug builds and won't provide production observability. Consider implementing a structured logging approach (if available in ThirdwebClient) or distinguish between expected null returns and actual errors by allowing specific exceptions to propagate.
🤖 Prompt for AI Agents
In Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs around lines 1701 to 1705,
the broad catch(Exception) silently returns null which hides production errors;
replace this with targeted handling: catch only the expected exceptions that
mean “no allowlist or no public mint” and return null for those, and otherwise
log the caught exception using the project's structured logging (use
ThirdwebClient's logger if available, or inject/obtain an ILogger) and rethrow
or wrap the exception; ensure you do not use Debug.WriteLine so that logs appear
in production, and include exception message and stack trace in the log entry
for diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (2)
1591-1643: Consider distinguishing between "no allowlist" and "public mint" scenarios.The method returns an identical empty
AllowlistProofstructure in two different cases:
- Line 1591-1601: When
metadata.Merkleis null or empty- Line 1632-1643: When the merkle root is zero (public mint)
However, it returns
nullfor other failure cases (lines 1662, 1670, 1692). This inconsistency may confuse callers who need to distinguish between "wallet is not allowlisted" vs "allowlist doesn't exist" vs "public mint".Consider either documenting this behavior clearly in XML comments, or making the return values more consistent.
1748-1773: LGTM with minor observation.The allowlist proof integration correctly handles different scenarios:
- Valid proof with elements → constructs allowlist proof payload
- Price override in allowlist → recalculates payable amount
- No proof or empty proof → falls back to empty structure
The logic appropriately checks for both
NATIVE_TOKEN_ADDRESSandADDRESS_ZEROwhen determining if the currency is native (line 1766).Note: This logic is duplicated in
DropERC721_Claim(lines 1910-1935) andDropERC1155_Claim(lines 2097-2122). Consider extracting this into a helper method to reduce duplication and ensure consistency across all three claim methods.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (5)
Thirdweb/Thirdweb.Utils/MerkleTree.cs (3)
AllowlistProof(95-220)MerkleTreeUtils(10-238)GetShardKey(228-237)Thirdweb.Tests/Thirdweb.Contracts/Thirdweb.Contracts.Tests.cs (1)
AllowlistProof(133-139)Thirdweb/Thirdweb.Utils/Utils.cs (4)
BigInteger(150-153)BigInteger(397-411)List(586-665)BytesToHex(119-122)Thirdweb/Thirdweb.Storage/ThirdwebStorage.cs (1)
ThirdwebStorage(8-106)Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.Types.cs (3)
ContractMetadata(54-86)MerkleTreeInfo(95-126)ShardData(131-144)
🔇 Additional comments (2)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (2)
1910-1935: Allowlist proof integration looks correct for ERC721.The implementation correctly:
- Fetches the allowlist proof for the receiver
- Constructs the proof payload when available
- Recalculates payable amount with price override from allowlist
- Handles both native and ERC20 payment currencies
- Falls back to empty proof structure when no allowlist data exists
2097-2122: ERC1155 allowlist proof integration correctly handles token-specific allowlists.The implementation appropriately passes
tokenIdtoGetAllowlistProof(line 2098), which is necessary for ERC1155 contracts where allowlists can be token-specific. The rest of the logic correctly mirrors the ERC721/ERC20 implementations.
…ability - Distinguish between expected 'wallet not in allowlist' (404 errors) and unexpected failures (network issues, auth errors) - Only 404/NotFound errors return null silently, other errors propagate up for proper debugging - Add IsNotFoundError helper function for cleaner exception filtering
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (3)
1683-1702: Exception handling improved but consider adding observability.The targeted exception handling is a significant improvement over the previous broad catch-all approach. The
IsNotFoundErrorhelper clearly defines expected failures (wallet not in allowlist), and unexpected errors (network issues, auth failures) now propagate to callers instead of being silently swallowed.However, while the propagation allows callers to handle errors, there's still no observability when unexpected errors occur. Consider adding structured logging before re-throwing unexpected exceptions to help diagnose production issues:
catch (Exception ex) when (IsNotFoundError(ex)) { // Try without .json extension (some IPFS gateways don't need it) try { shardUri = $"{treeInfo.BaseUri}/{shardKey}"; shardData = await ThirdwebStorage.Download<ShardData>(contract.Client, shardUri).ConfigureAwait(false); } catch (Exception ex2) when (IsNotFoundError(ex2)) { // Shard not found - wallet is not in the allowlist (expected case) return null; } + catch (Exception ex2) + { + // Log unexpected error: ex2.Message, ex2.GetType(), shardUri + throw; + } // Other errors (network, auth, etc.) propagate up as unexpected }Based on learnings from past review comments, this addresses the observability concerns while maintaining the improved exception handling pattern.
1769-1776: Consider caching the parsed MAX_UINT256 value.The allowlist price override logic is correct, but
BigInteger.Parse(Constants.MAX_UINT256_STR)is called repeatedly across three claim methods (DropERC20, DropERC721, DropERC1155).Consider adding a static readonly field to avoid repeated parsing:
+private static readonly BigInteger MAX_UINT256 = BigInteger.Parse(Constants.MAX_UINT256_STR); // Recalculate payable amount if allowlist has price override -var maxUint256 = BigInteger.Parse(Constants.MAX_UINT256_STR); -if (allowlistProofData.PricePerToken < maxUint256) +if (allowlistProofData.PricePerToken < MAX_UINT256) { // ... }This would eliminate redundant parsing and improve performance slightly, though the impact is minimal.
Also applies to: 1931-1938, 2118-2125
1560-1575: Clarify documentation on return values.The XML documentation states the method returns "null if the wallet is not in the allowlist or it's a public mint," but the implementation returns an empty
AllowlistProoffor public mint scenarios (lines 1593-1600, 1636-1642) andnullonly when:
- Wallet is not in the allowlist (shard not found, line 1698)
- Merkle root is not found in metadata (line 1662)
The second null case (merkle root not found) might indicate a configuration error rather than "wallet not in allowlist," but callers cannot distinguish between these cases.
Consider clarifying the documentation:
/// <returns>The allowlist proof, or null if the wallet is not in the allowlist or it's a public mint.</returns> +/// <returns>An AllowlistProof with an empty proof array for public mints, or null if the wallet is not in the allowlist or merkle data is unavailable.</returns>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs(4 hunks)
🔇 Additional comments (1)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (1)
1756-1781: Allowlist proof integration looks correct.The integration of
GetAllowlistProofinto the claim methods correctly:
- Fetches the allowlist proof for the receiver address
- Constructs the proof payload when valid proof data is returned
- Recalculates
payableAmountwhen the allowlist has a price override (PricePerToken < MAX_UINT256)- Handles both native token and ERC20 currency scenarios
- Falls back to empty proof when no allowlist data is available
The absence of error handling around
GetAllowlistProofmeans unexpected errors (network failures, invalid IPFS data) will propagate and cause the claim transaction to fail. This is acceptable behavior since claiming without valid allowlist data could lead to incorrect pricing or unauthorized claims.Also applies to: 1918-1943, 2105-2130
- Add static readonly MaxUint256 field to avoid repeated BigInteger.Parse() calls - Clarify GetAllowlistProof return value documentation (empty proof for public mint vs null for not in allowlist)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (1)
1682-1710: Exception handling correctly distinguishes expected vs. unexpected failures.The
IsNotFoundErrorhelper and subsequent exception handling properly separate expected scenarios (wallet not in allowlist) from unexpected errors. When a shard is not found (404/NotFound), the method returnsnullas documented. Other exceptions propagate, preserving observability for network failures, authorization issues, or invalid URIs.The fallback logic (lines 1694-1699) that retries without the
.jsonextension accommodates different IPFS gateway behaviors, which is a pragmatic approach.Consider adding a brief comment above the
IsNotFoundErrorhelper explaining why string matching is used and what the expected error messages are:+// Helper to identify "not found" responses from storage layer (IPFS gateway returns 404 or NotFound in message) static bool IsNotFoundError(Exception ex) => ex.Message.Contains("NotFound", StringComparison.OrdinalIgnoreCase) || ex.Message.Contains("404", StringComparison.Ordinal);This would help future maintainers understand the dependency on exception message text.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (4)
Thirdweb/Thirdweb.Utils/Constants.cs (1)
Constants(3-56)Thirdweb/Thirdweb.Utils/MerkleTree.cs (3)
AllowlistProof(95-220)MerkleTreeUtils(10-238)GetShardKey(228-237)Thirdweb/Thirdweb.Storage/ThirdwebStorage.cs (1)
ThirdwebStorage(8-106)Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.Types.cs (3)
ContractMetadata(54-86)MerkleTreeInfo(95-126)ShardData(131-144)
🔇 Additional comments (4)
Thirdweb/Thirdweb.Extensions/ThirdwebExtensions.cs (4)
9-10: LGTM! Good use of constant for MAX_UINT256 sentinel value.The
MaxUint256field properly encapsulates the max uint256 value used as a sentinel to detect price overrides in allowlist proofs. The field is correctly scoped asprivate static readonlyand initialized from the constant defined inConstants.MAX_UINT256_STR.
1576-1677: Well-structured allowlist proof retrieval with proper null handling.The
GetAllowlistProofmethod implements a robust multi-step flow:
- Validates parameters and fetches contract metadata
- Distinguishes between public mint (returns empty proof) and allowlist mint scenarios
- Flexibly handles optional
claimConditionIdandtokenIdparameters for ERC20/721/1155 support- Implements case-insensitive merkle root lookup as a fallback (line 1657), which handles hex string case variations
- Returns
nullwhen wallet is not in allowlist or merkle data is missing, letting the contract enforce validationThe clear separation between empty proof (public mint) and
null(not in allowlist) is well-documented and correctly handled downstream in the claim methods.
1761-1785: Allowlist proof integration correctly handles price overrides and native token detection.The integration with
GetAllowlistProofproperly:
- Validates the returned proof data has actual entries before using it
- Constructs the allowlist proof structure with all required fields (proof array, quantity limit, price, currency)
- Detects price overrides by comparing against
MaxUint256sentinel value (line 1775)- Recalculates
payableAmountwhen an allowlist-specific price is present- Checks both
NATIVE_TOKEN_ADDRESSandADDRESS_ZEROfor native currency (line 1778), accommodating different conventionsThe fallback to an empty proof (lines 1782-1784) when
GetAllowlistProofreturnsnullor an empty proof is correct - the smart contract enforces allowlist validation, so the SDK doesn't need to fail early.
1922-1946: Consistent allowlist integration across ERC721 and ERC1155 claim methods.Both
DropERC721_ClaimandDropERC1155_Claimfollow the same integration pattern asDropERC20_Claim:ERC721 (lines 1922-1946):
- Uses
quantity * pricePerTokenfor payable amount calculation (no decimals adjustment needed)- Identical proof handling logic
ERC1155 (lines 2108-2132):
- Correctly passes
tokenIdtoGetAllowlistProof(line 2109) since ERC1155 drops have per-token claim conditions- Otherwise mirrors the ERC721 implementation
The consistency across all three drop types makes the codebase easier to maintain and reduces the likelihood of bugs.
Also applies to: 2108-2132
2 participants
PR-Codex overview
This PR focuses on enhancing the Merkle tree functionality and related structures within the
Thirdwebproject, particularly for managing allowlist claims in token drops.Detailed summary
global.jsonSDK version and rollForward settings.MAX_UINT256_STRconstant inConstants.cs.ThirdwebExtensions.Types.csfor Merkle tree handling.MerkleTreeUtilsfor Merkle proof calculations inMerkleTree.cs.ThirdwebExtensionswith allowlist proof retrieval methods.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.