Pass final recipient to NFTCollection for correct limit calculation #1736
Conversation
WalkthroughAdds purchase-agent–mediated minting: INFTCollection gains NotPurchaseAgent and wrappedWaveMint; NFTCollection implements wrappedWaveMint with purchaseAgent checks and payment flow; PurchaseWrapper threads a finalReceiver into confirmPurchase and calls wrappedWaveMint; tests, deployment, and Hardhat named accounts updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Wrapper as PurchaseWrapper
participant ERC20 as SAND Token
participant Collection as NFTCollection
participant Treasury as Mint Treasury
User->>Wrapper: confirmPurchase(sender, finalReceiver, collection, waveIndex, signatureId, localTokenId?, signature)
Wrapper->>ERC20: transferFrom(sender -> Wrapper, price)
alt price > 0
ERC20-->>Wrapper: funds received
end
note over Wrapper,Collection: Wrapper is configured as purchaseAgent on Collection
Wrapper->>Collection: wrappedWaveMint(caller=Wrapper, destinationWallet=finalReceiver, amount=1, waveIndex, signatureId, signature)
Collection->>Collection: validate purchaseAgent, wave config, signer, per-wallet limits
Collection->>Treasury: transfer price (if applicable)
Collection->>Collection: update counters, totalSupply, mint tokens
Collection-->>Wrapper: return minted token IDs
Wrapper-->>User: emit PurchaseConfirmed, proceed with transfer/delivery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 3
🧹 Nitpick comments (10)
packages/deploy/hardhat.config.ts (1)
105-113: Verify deployer EOAs and consider centralizing via env/named constantsThese are production-impacting EOAs. Please confirm ownership/rotation plans and that they’re correct for each network. Consider moving the repeated addresses to env-driven constants (or a small mapping) to avoid future drift.
packages/avatar/contracts/nft-collection/NFTCollection.sol (1)
298-303: Optional: emit destination wallet for observabilityWaveMint logs caller only. Consider adding a dedicated event (or extending the existing one) to include destinationWallet for off-chain reconciliation.
packages/avatar/contracts/nft-collection/INFTCollection.sol (1)
359-376: Clarify who actually receives the minted token and how auth is enforcedDocs say “to a destination wallet,” while PurchaseWrapper’s call/comment suggests tokens mint to the wrapper (caller) and only limits are accounted against destinationWallet. Also, since calls come via SAND.approveAndCall, msg.sender is the SAND token; authorization likely checks that:
- msg.sender == allowedToExecuteMint
- caller == purchaseAgent
Recommend tightening NatSpec to avoid ambiguity.
Apply doc-only diff:
- * @notice Mints a token for a specific wave to a destination wallet. - * @dev Can only be called by the purchaseAgent. + * @notice Mints for a specific wave while accounting per-wallet limits against `destinationWallet`. + * @dev Must be invoked by the payment token via approveAndCall (i.e., `msg.sender == allowedToExecuteMint`), + * and `caller` must equal the configured `purchaseAgent`. + * Implementation mints the token(s) to `caller` (the purchase agent), while quota/limit checks + * are performed against `destinationWallet`.packages/avatar/contracts/nft-collection/PurchaseWrapper.sol (4)
153-160: Consider emitting finalReceiver for traceabilityPurchaseConfirmed currently omits finalReceiver. Emitting it would help correlate quota accounting vs. actual recipient of the NFT.
If desired, extend the event and emit it here. I can draft the minimal ABI-safe upgrade (new event) if you want.
243-261: Align comment with actual semanticsThe inline comment “NFTs will be minted to this contract first” is the critical behavior enabling post-mint transfer to the buyer. Keep this comment, but consider adding that destinationWallet is used for limit checks in the collection, not as the mint recipient.
- address(this), // NFTs will be minted to this contract first + address(this), // NFTs mint to the wrapper; collection uses `finalReceiver` for per-wallet limits finalReceiver,
271-293: Prefer abi.decode over manual offset parsing for nested return dataManual offsets are brittle to encoding changes. Decode the nested bytes safely.
Apply this diff:
- uint256[] memory tokenIds = new uint256[](1); - // The return data from `approveAndCall` is a `bytes` type, which means the actual return data from `waveMint` (an abi-encoded uint256[]) - // is itself abi-encoded. We need to go deeper. - // `result` raw data layout: - // - 0x00: offset to bytes data (0x20) - // - 0x20: length of bytes data (e.g., 96 for a single uint256 in an array) - // - 0x40: start of the `waveMint` return data - // - 0x40: offset to array data (0x20) - // - 0x60: array length (1) - // - 0x80: the token ID - if (result.length < 160) { - revert PurchaseWrapperNftPurchaseFailedViaApproveAndCall(); - } - bytes32 tokenIdWord; - assembly { - // We read the word at offset 0x80 in the raw return data. - // The `result` variable is a memory pointer, and its data starts at an offset of 0x20. - // So we read from result + 0x20 (start of data) + 0x80 (offset to tokenId) = result + 0xa0 - tokenIdWord := mload(add(result, 0xa0)) - } - tokenIds[0] = uint256(tokenIdWord); - return tokenIds[0]; + // `approveAndCall` returns bytes; inner payload is the collection's abi-encoded uint256[]. + bytes memory inner = abi.decode(result, (bytes)); + uint256[] memory tokenIds = abi.decode(inner, (uint256[])); + if (tokenIds.length == 0) revert PurchaseWrapperNftPurchaseFailedViaApproveAndCall(); + return tokenIds[0];
300-301: Misleading error payload: it reports the EOA ‘sender’, not msg.senderThe revert passes
senderinto PurchaseWrapperCallerNotAuthorized(address), which reads as the caller not authorized, but the caller is msg.sender (SAND or a direct EOA). Consider either renaming the error or including both addresses for clarity.Example:
-error PurchaseWrapperCallerNotAuthorized(address caller); +error PurchaseWrapperCallerNotAuthorized(address sandMsgSender, address originalSender); ... - revert PurchaseWrapperCallerNotAuthorized(sender); + revert PurchaseWrapperCallerNotAuthorized(msg.sender, sender);packages/avatar/test/avatar/nft-collection/PurchaseWrapper.test.ts (3)
114-123: Nice coverage of finalReceiver threading; add an assertion once to prove quota is tracked against itYou already assert quota in the multi-recipient test. For completeness, add a single check (e.g., after the first confirmPurchase) that waveOwnerToClaimedCounts(waveIndex, finalReceiver) == 1.
If helpful, I can draft a minimal assertion snippet to drop into the first test.
Also applies to: 145-155, 209-210, 234-245, 286-294, 543-554, 673-674
326-333: Fix typos and stale comments in the multi-recipient test
- Title: “ming” -> “mint”.
- The “Second Purchase (should fail)” comment contradicts the expectation; it should say “should succeed”.
Apply this diff:
-it('should allow two different users to ming through purchase wrapper when max tokens per wallet is set to 1', async function () { +it('should allow two different users to mint through purchase wrapper when max tokens per wallet is set to 1', async function () { ... - // --- Second Purchase (should fail) --- + // --- Second Purchase (should succeed) ---Also applies to: 381-407
520-522: Add a test for zero-address finalReceiver once wrapper enforces itAfter adding the zero-address guard, include a revert test to lock it in.
Example:
+ it('should revert when finalReceiver is zero address', async function () { + const {collectionContractAddress, sandContract, randomWallet: userA, purchaseWrapper, purchaseWrapperAddress} = + await loadFixture(setupPurchaseWrapperFixture); + const sandPrice = ethers.parseEther('100'); + const waveIndex = 0; + const signatureId = 999; + const randomTempTokenId = 888; + const userAAddress = await userA.getAddress(); + await sandContract.donateTo(userAAddress, sandPrice); + await sandContract.connect(userA).approve(purchaseWrapperAddress, sandPrice); + const signature = '0x'; + const data = purchaseWrapper.interface.encodeFunctionData('confirmPurchase', [ + userAAddress, + ethers.ZeroAddress, + collectionContractAddress, + waveIndex, + signatureId, + randomTempTokenId, + signature + ]); + await expect( + sandContract.connect(userA).approveAndCall(purchaseWrapperAddress, sandPrice, data) + ).to.be.revertedWithCustomError(purchaseWrapper, 'PurchaseWrapperInvalidRecipientAddress'); + });Also applies to: 544-554
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
packages/avatar/contracts/nft-collection/INFTCollection.sol(2 hunks)packages/avatar/contracts/nft-collection/NFTCollection.sol(1 hunks)packages/avatar/contracts/nft-collection/PurchaseWrapper.sol(3 hunks)packages/avatar/test/avatar/fixtures.ts(3 hunks)packages/avatar/test/avatar/nft-collection/PurchaseWrapper.test.ts(22 hunks)packages/deploy/deploy/28_nft_collection/04_deploy_purchase_wrapper.ts(1 hunks)packages/deploy/hardhat.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: deploy
- GitHub Check: analyze
- GitHub Check: coverage
- GitHub Check: lint
- GitHub Check: format
- GitHub Check: test
🔇 Additional comments (8)
packages/avatar/test/avatar/fixtures.ts (2)
3-5: LGTM: ethers imports consolidated for v6Imports are correct and consistent with usage (parseUnits BigInt).
49-64: LGTM: exposing randomWallet3Keeps fixtures aligned with multi-recipient tests.
packages/avatar/contracts/nft-collection/NFTCollection.sol (3)
275-277: Tighten auth errors and split checks for clarityReturn precise errors and avoid misreporting token as “not purchase agent”.
[ suggest_essential_refactor ]
Apply this diff:- if (_msgSender() != address($.allowedToExecuteMint) || caller != $.purchaseAgent) { - revert NotPurchaseAgent(_msgSender()); - } + if (_msgSender() != address($.allowedToExecuteMint)) { + revert ERC721InvalidSender(_msgSender()); + } + if (caller != $.purchaseAgent) { + revert NotPurchaseAgent(caller); + }
289-301: Ensure mint recipient aligns with accounting wallet
wrappedWaveMint mints tocallerbut updates counts underdestinationWallet, and PurchaseWrapper.confirmPurchase calls transferFrom(address(this), sender, tokenId), delivering the NFT tosenderinstead of thefinalReceiverargument. If the NFT should land atfinalReceiver, update the transfer to usefinalReceiverand add a test asserting its ownership.
281-283: Incorrect — signature already binds destination walletwaveMint calls _checkAndSetWaveMintSignature(wallet, waveIndex, signatureId, signature) and NFTCollectionSignature._getWaveMintSignature includes the wallet in the signed payload; the proposed caller→destinationWallet diff is unnecessary.
Likely an incorrect or invalid review comment.
packages/avatar/contracts/nft-collection/INFTCollection.sol (1)
322-327: Good addition: explicit error for purchase-agent violationsThe dedicated NotPurchaseAgent(address) error is clear and helps distinguish auth failures from other errors. No issues.
packages/avatar/test/avatar/nft-collection/PurchaseWrapper.test.ts (2)
32-35: Required wiring: setting PurchaseAgent to the wrapper looks correctThis mirrors the on-chain requirement for the wrappedWaveMint path. Good.
571-654: Good negative path: enforcing per-wallet max via finalReceiverThis validates that using the same finalReceiver twice reverts via the wrapper surface. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/avatar/contracts/nft-collection/PurchaseWrapper.sol (3)
13-20: Docs: “Uses Ownable” is incorrect; contract uses AccessControlUpdate header doc to match implementation.
296-304: Authorization gate is wrong: confirmPurchase will always revert unless called by SANDCurrently requires msg.sender == SAND and ‘sender’ to hold the role; confirmPurchase is invoked by a purchase agent/backend, not SAND.
Apply:
- if (msg.sender != address(sandToken) || !hasRole(AUTHORIZED_CALLER_ROLE, sender)) { - revert PurchaseWrapperCallerNotAuthorized(sender); - } + if (!hasRole(AUTHORIZED_CALLER_ROLE, msg.sender)) { + revert PurchaseWrapperCallerNotAuthorized(msg.sender); + }
308-311: TOCTOU on localTokenId: collision possible before mint completesOnly checking nftTokenId leaves a window where a second call can reuse the same ID before the first mints.
Apply:
- if (_purchaseInfo[randomTempTokenId].nftTokenId != 0) { + PurchaseInfo memory existing = _purchaseInfo[randomTempTokenId]; + if (existing.caller != address(0) || existing.nftTokenId != 0) { revert PurchaseWrapperLocalTokenIdAlreadyInUse(randomTempTokenId); }
♻️ Duplicate comments (1)
packages/avatar/contracts/nft-collection/PurchaseWrapper.sol (1)
142-142: Good: zero-address validation for finalReceiverThis addresses the earlier review about zero-address recipients.
🧹 Nitpick comments (4)
packages/avatar/contracts/nft-collection/PurchaseWrapper.sol (4)
118-124: Docs mention _txContext and onERC721Received flow that no longer existPrune outdated references; describe the current approveAndCall → wrappedWaveMint path and post-call transfer.
132-140: Guard sender != address(0)Prevent storing a zero “original sender”.
Apply:
function confirmPurchase( address sender, address finalReceiver, address nftCollection, uint256 waveIndex, uint256 signatureId, uint256 randomTempTokenId, bytes calldata signature ) external nonReentrant { + if (sender == address(0)) revert PurchaseWrapperInvalidRecipientAddress();
253-262: Prefer abi.decode over manual length check + assembly for nested returnSafer and clearer.
Apply:
- uint256[] memory tokenIds = new uint256[](1); - if (result.length < 160) { - revert PurchaseWrapperNftPurchaseFailedViaApproveAndCall(); - } - bytes32 tokenIdWord; - assembly { - tokenIdWord := mload(add(result, 0xa0)) - } - tokenIds[0] = uint256(tokenIdWord); - return tokenIds[0]; + bytes memory inner = abi.decode(result, (bytes)); + uint256[] memory tokenIds = abi.decode(inner, (uint256[])); + if (tokenIds.length != 1) revert PurchaseWrapperNftPurchaseFailedViaApproveAndCall(); + return tokenIds[0];Also applies to: 272-293
61-74: Event lacks finalReceiver (useful for auditing per-wallet limits)Consider indexing finalReceiver in PurchaseConfirmed for traceability.
I can propose an ABI-safe event change if backward-compat isn’t required, or a new event otherwise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/avatar/contracts/nft-collection/PurchaseWrapper.sol(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: analyze
- GitHub Check: format
- GitHub Check: coverage
- GitHub Check: deploy
🔇 Additional comments (5)
packages/avatar/contracts/nft-collection/PurchaseWrapper.sol (5)
153-161: Plumbing finalReceiver through mint path looks correctArgument order and usage align with the new API.
164-167: Confirm intended recipient: transfer goes to sender, not finalReceiverIf finalReceiver is only for quota calculation, rename for clarity (e.g., limitWallet). If it should be the recipient, change this transfer.
Would you like a patch to deliver to finalReceiver and persist it in PurchaseInfo?
245-251: Signature change carried through correctlyPrivate helper accepts finalReceiver; no issues spotted.
253-262: Comment may be misleading about mint destination“NFTs will be minted to this contract first” might be false if collection mints to destinationWallet. Update comment or logic accordingly.
Apply:
- address(this), // NFTs will be minted to this contract first + address(this), // purchase agent identity; mint destination is decided by the collection
177-192: Permission model vs docs: anyone with AUTHORIZED_CALLER_ROLE can move user NFTsDocs say “msg.sender must be the from address,” but code doesn’t enforce it. Either enforce or update docs.
If enforcement is desired, I can add a new specific error and the require(msg.sender == from) check.
Description
Summary by CodeRabbit
New Features
Tests
Chores