Formalize auction custody and settlement#59
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR implements the P0-AUCT-001 auction custody and settlement state machine (ADR 0002), introducing an escrow-based custody model where auction NFTs are held by the auction contract from creation through settlement, a canonical ChangesAuction Custody and Settlement Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@claude please review this PR, with special attention to auction custody/state transitions, with-bid settlement atomicity, no-bid contract-poster claim semantics, and whether the docs/test matrix accurately describe the remaining non-goals. |
|
@coderabbitai review latest head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the PR diff and key changed files
gh pr view 59 --repo 6529-Collections/6529Stream --json title,body,files,headRefName,baseRefName | jq '{title, headRefName, baseRefName, files: [.files[].path]}'Length of output: 1016 🏁 Script executed: #!/bin/bash
# Get the full diff for the PR at the specified commit
git diff origin/main...7df147605729f90f9dd80a86d47a969bd09051e6 --statLength of output: 1255 🏁 Script executed: #!/bin/bash
# Read the main auction contract
cat smart-contracts/AuctionContract.sol 2>/dev/null || echo "FILE NOT FOUND"Length of output: 18558 🏁 Script executed: #!/bin/bash
# Read IStreamAuctions interface
cat smart-contracts/IStreamAuctions.sol 2>/dev/null || echo "FILE NOT FOUND"Length of output: 471 🏁 Script executed: #!/bin/bash
# Read StreamDrops changes related to auction
cat smart-contracts/StreamDrops.sol 2>/dev/null || echo "FILE NOT FOUND"Length of output: 17737 🏁 Script executed: #!/bin/bash
# Read the custody test file
cat test/StreamAuctionCustody.t.sol 2>/dev/null || echo "FILE NOT FOUND"Length of output: 10714 🏁 Script executed: #!/bin/bash
# Read the payments test file
cat test/StreamAuctionPayments.t.sol 2>/dev/null || echo "FILE NOT FOUND"Length of output: 18168 🏁 Script executed: #!/bin/bash
# Read docs - ADR 0002
fd -e md . docs/ | xargs grep -l "0002\|auction" 2>/dev/null | head -10Length of output: 437 🏁 Script executed: #!/bin/bash
# Read ADR files
fd "ADR\|adr" docs/ -e md | head -10
cat docs/adr/0002-auction-custody.md 2>/dev/null || fd "0002" docs/ | head -5 | xargs cat 2>/dev/null || echo "ADR 0002 not found at expected path"Length of output: 17720 🏁 Script executed: #!/bin/bash
cat docs/auction-custody.md 2>/dev/null || echo "FILE NOT FOUND"Length of output: 4661 🏁 Script executed: #!/bin/bash
cat docs/known-blockers.md 2>/dev/null || echo "FILE NOT FOUND"Length of output: 1477 🏁 Script executed: #!/bin/bash
cat ops/ROADMAP.md 2>/dev/null || echo "FILE NOT FOUND"Length of output: 89392 🏁 Script executed: #!/bin/bash
cat test/README.md 2>/dev/null || echo "FILE NOT FOUND"Length of output: 1958 🏁 Script executed: #!/bin/bash
cat test/mocks/MockStreamAuctions.sol 2>/dev/null || echo "FILE NOT FOUND"Length of output: 946 🏁 Script executed: #!/bin/bash
# Check the full diff to see what changed in StreamDropsCharacterization and StreamDropsIntegrationCharacterization
git diff origin/main...7df147605729f90f9dd80a86d47a969bd09051e6 -- test/StreamDropsCharacterization.t.sol test/StreamDropsIntegrationCharacterization.t.sol test/StreamDropsERC1271.t.solLength of output: 9809 🏁 Script executed: #!/bin/bash
# Check the actual diff for AuctionContract.sol and StreamDrops.sol
git diff origin/main...7df147605729f90f9dd80a86d47a969bd09051e6 -- smart-contracts/AuctionContract.sol smart-contracts/StreamDrops.sol smart-contracts/IStreamAuctions.solLength of output: 20805 🏁 Script executed: #!/bin/bash
# Check ADR 0003 and ops/ROADMAP changes
git diff origin/main...7df147605729f90f9dd80a86d47a969bd09051e6 -- docs/adr/0003-payment-accounting.md ops/AUTONOMOUS_RUN.mdLength of output: 12323 🏁 Script executed: #!/bin/bash
# Check for the ReentrancyGuard implementation
cat smart-contracts/ReentrancyGuard.sol 2>/dev/null || echo "FILE NOT FOUND"Length of output: 3419 🏁 Script executed: #!/bin/bash
# Check IERC721 and IERC721Receiver implementations
cat smart-contracts/IERC721.sol 2>/dev/null
cat smart-contracts/IERC721Receiver.sol 2>/dev/nullLength of output: 6043 🏁 Script executed: #!/bin/bash
# Look for any proceedsCredit rounding or dust tracking
rg "dust\|rounding\|totalPosterOwed\|totalProtocolOwed\|totalCuratorOwed\|posterCredit\|protocolCredit\|curatorCredit" smart-contracts/AuctionContract.solLength of output: 465 🏁 Script executed: #!/bin/bash
# Check test helpers
cat test/helpers/StreamFixture.sol 2>/dev/null || echo "FILE NOT FOUND"
cat test/helpers/DropAuthTestHelper.sol 2>/dev/null | head -100Length of output: 6059
Auction custody & state transitionsThe
With-bid settlement atomicityThe CEI order in If No-bid contract-poster claim semanticsThe two-phase flow is correct:
Missing test — failed NFT transfer in function testNoBidContractPosterClaimFailureRollsBack() public {
NonReceiverPoster poster = new NonReceiverPoster();
AuctionSetup memory setup = _createAuction(address(poster), block.timestamp + 1 days, N);
vm.warp(setup.auctionEndTime + 1);
setup.auctions.claimAuction(setup.tokenId); // → sets pendingNoBidNftClaimant
NonReceiverBidder rejectingRecipient = new NonReceiverBidder(); // rejects ERC-721
(bool success,) = address(poster).call(
abi.encodeWithSelector(
poster.claim.selector, setup.auctions, setup.tokenId, address(rejectingRecipient)
)
);
success.assertFalse("claim to rejecting recipient succeeded");
setup.deployed.core.ownerOf(setup.tokenId)
.assertEq(address(setup.auctions), "token left escrow");
setup.auctions.pendingNoBidNftClaimant(setup.tokenId)
.assertEq(address(poster), "pending claim cleared");
uint256(setup.auctions.retrieveAuctionStatus(setup.tokenId))
.assertEq(uint256(StreamAuctions.AuctionStatus.EndedNoBid), "status advanced");
}Proceeds split — undocumented roundinguint256 posterCredit = _highestBid / 2;
uint256 protocolCredit = _highestBid / 4;
uint256 curatorCredit = _highestBid - posterCredit - protocolCredit;The curator absorbs any integer remainder (1–2 wei for non-4-divisible bids). Minor findings
Forced-ETH test not present. ADR 0002 test plan lists "forced ETH does not corrupt owed/surplus views" as required.
Docs / test matrix accuracyADR 0002 status block, ADR 0003 diff, Summary
The core implementation is sound. The one item I'd prioritize before merge is the missing rollback test for |
![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 (1)
smart-contracts/AuctionContract.sol (1)
381-405: 💤 Low valueConsider validating
payOutAddressandcuratorsPoolAddressare non-zero before crediting.If either address is
address(0)(e.g., due to admin misconfiguration), proceeds credited to them become unclaimable. The admin update functions (updatePayOutAddress,updateCuratorsPoolAddress) don't validate against zero addresses.This is a low-probability misconfiguration scenario given admin controls, but for defense-in-depth you could add a check here or in the update functions.
🛡️ Optional: Add zero-address validation
function _creditAuctionProceeds(uint256 _tokenid, address _poster, uint256 _highestBid) private { + require(payOutAddress != address(0), "No payout address"); + require(curatorsPoolAddress != address(0), "No curator address"); uint256 posterCredit = _highestBid / 2;Or alternatively in the update functions:
function updatePayOutAddress(address _payOutAddress) public FunctionAdminRequired(this.updatePayOutAddress.selector) { + require(_payOutAddress != address(0), "Zero address"); payOutAddress = _payOutAddress; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@smart-contracts/AuctionContract.sol` around lines 381 - 405, The _creditAuctionProceeds function currently credits payOutAddress and curatorsPoolAddress without verifying they are not address(0); add validation to prevent credits to the zero address by requiring payOutAddress != address(0) and curatorsPoolAddress != address(0) before performing credits (use require with clear error messages), or alternatively add the same non-zero checks inside the admin setters updatePayOutAddress and updateCuratorsPoolAddress to block setting zero addresses; reference _creditAuctionProceeds, payOutAddress, curatorsPoolAddress, updatePayOutAddress and updateCuratorsPoolAddress when implementing the fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/auction-custody.md`:
- Around line 122-123: The doc uses inconsistent casing `tokenid`; update all
occurrences to `tokenId` (e.g., in the sentence about indexers and examples) so
it matches the API and other docs; ensure the guidance references `tokenId` plus
`dropId` from the registration event and calls `retrieveAuctionStatus(tokenId)`
consistently everywhere in this document.
---
Nitpick comments:
In `@smart-contracts/AuctionContract.sol`:
- Around line 381-405: The _creditAuctionProceeds function currently credits
payOutAddress and curatorsPoolAddress without verifying they are not address(0);
add validation to prevent credits to the zero address by requiring payOutAddress
!= address(0) and curatorsPoolAddress != address(0) before performing credits
(use require with clear error messages), or alternatively add the same non-zero
checks inside the admin setters updatePayOutAddress and
updateCuratorsPoolAddress to block setting zero addresses; reference
_creditAuctionProceeds, payOutAddress, curatorsPoolAddress, updatePayOutAddress
and updateCuratorsPoolAddress when implementing the fixes.
🪄 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: 1ef1ddc0-29cf-4ab5-973d-27403f630744
📒 Files selected for processing (16)
docs/adr/0002-auction-custody.mddocs/adr/0003-payment-accounting.mddocs/auction-custody.mddocs/known-blockers.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdsmart-contracts/AuctionContract.solsmart-contracts/IStreamAuctions.solsmart-contracts/StreamDrops.soltest/README.mdtest/StreamAuctionCustody.t.soltest/StreamAuctionPayments.t.soltest/StreamDropsCharacterization.t.soltest/StreamDropsERC1271.t.soltest/StreamDropsIntegrationCharacterization.t.soltest/mocks/MockStreamAuctions.sol
|
Review-fix commit pushed: Addressed the CodeRabbit review items:
Latest local validation:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the diff for the fix commit
git show 1e52d6dfaf8a61bc7066ee3fade144bf4a397488 --statLength of output: 1096 🏁 Script executed: #!/bin/bash
# Check the full diff of the fix commit
git show 1e52d6dfaf8a61bc7066ee3fade144bf4a397488 --no-colorLength of output: 26209
The two bonus items (zero-address constructor/setter guards with Implementation is sound. No new issues observed in the fix commit. ✓ [approve] |
|
Autonomous merge decision for PR #59:
Proceeding to merge under the autonomous maintainer rules. |
1 participant
Summary
Closes #22.
This implements the accepted ADR 0002 auction-custody target behavior for
P0-AUCT-001, building on PR #58's outbid pull-credit work.StreamDropsto use a configured auction contract before auction drops mintStreamAuctionsERC-721 receiver support, canonical status views, terminal states, end-time views, and pending no-bid claim viewsActive, keep auction-local end-time extension in the auction record, and preserve bidder-credit accountingReview-Fix Additions
Created -> Activecustody path and legacy minter end-time divergence after auction extensionStreamDropsaccepts a configured auction contractAuctionRecord.tokenIdto match ADR storage shapeForceEthhelpertokenIdcasing in indexer guidanceNon-goals
This PR intentionally does not finish the broader payment/accounting roadmap:
Validation
forge test --match-contract 'StreamAuction(Custody|Payments)Test|StreamDropsCharacterizationTest' -vvvpassed with 35 testsmake checkpassed with 85 tests and known compiler/NatSpec/lint warningspowershell -ExecutionPolicy Bypass -File scripts\check.ps1passed with 85 tests and known compiler/NatSpec/lint warningsforge fmt --check smart-contracts\AuctionContract.sol smart-contracts\StreamDrops.sol smart-contracts\IStreamAuctions.sol test\StreamAuctionCustody.t.sol test\StreamAuctionPayments.t.sol test\StreamDropsCharacterization.t.sol test\mocks\MockStreamAuctions.solpassed after adding Foundry toPATHgit diff --checkpassed.venv-tools\Scripts\slither.exewith Foundry onPATHreturned the expected non-zero baseline-finding exit: 10 High, 25 Medium, 53 Low, 511 Informational, 6 Optimization; noreentrancy-no-ethfindings and no production auction emergencyarbitrary-send-ethfinding on this deltaForceEth.forceselfdestructhelper used to prove forced ETH does not corrupt owed/surplus accountingReview notes
Please review especially the custody/state transitions in
smart-contracts/AuctionContract.sol, theStreamDropsauction-contract registration path, and the target-state coverage intest/StreamAuctionCustody.t.solandtest/StreamAuctionPayments.t.sol.Summary by CodeRabbit
New Features
Documentation
Tests