Cleanup, fix tests, config update #691
Conversation
WalkthroughThe constant Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
contracts/prebuilts/drop/DropERC721C.sol (1)
302-325: Add an explicit guard to prevent aggregate fees > MAX_BPS (clearer failure than underflow)If
platformFeeBpsis misconfigured so thatplatformFeeBps + DEFAULT_FEE_BPS > MAX_BPS, the final transfer math will revert due to underflow. Fail fast with a clear error.Apply near Line 304 (after reading platform fee info):
(address platformFeeRecipient, uint16 platformFeeBps) = getPlatformFeeInfo(); +require(uint256(platformFeeBps) + uint256(DEFAULT_FEE_BPS) <= MAX_BPS, "fees exceed MAX_BPS");
🧹 Nitpick comments (1)
contracts/prebuilts/drop/DropERC721C.sol (1)
306-325: Nit: variable naming — preferprotocolFeeandplatformFeefor clarity
platformFeesTwisn’t immediately obvious. UsingprotocolFee(for the default/protocol take) andplatformFee(for the configurable take) improves readability.- uint256 platformFeesTw = (totalPrice * DEFAULT_FEE_BPS) / MAX_BPS; - uint256 platformFees = (totalPrice * platformFeeBps) / MAX_BPS; + uint256 protocolFee = (totalPrice * DEFAULT_FEE_BPS) / MAX_BPS; + uint256 platformFee = (totalPrice * platformFeeBps) / MAX_BPS; - CurrencyTransferLib.transferCurrency(_currency, _msgSender(), DEFAULT_FEE_RECIPIENT, platformFeesTw); - CurrencyTransferLib.transferCurrency(_currency, _msgSender(), platformFeeRecipient, platformFees); + CurrencyTransferLib.transferCurrency(_currency, _msgSender(), DEFAULT_FEE_RECIPIENT, protocolFee); + CurrencyTransferLib.transferCurrency(_currency, _msgSender(), platformFeeRecipient, platformFee); CurrencyTransferLib.transferCurrency( _currency, _msgSender(), saleRecipient, - totalPrice - platformFees - platformFeesTw + totalPrice - platformFee - protocolFee );
| @@ -71,7 +71,7 @@ contract DropERC721C is | |||
| uint256 private constant MAX_BPS = 10_000; | |||
|
|
|||
| address public constant DEFAULT_FEE_RECIPIENT = 0x1Af20C6B23373350aD464700B5965CE4B0D2aD94; | |||
| uint16 private constant DEFAULT_FEE_BPS = 100; | |||
| uint16 private constant DEFAULT_FEE_BPS = 50; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Default protocol fee reduced to 50 bps — verify downstream assumptions and update docs/tests
This halves the protocol take-rate (1% → 0.5%). Please confirm no tests, deployment configs, UI copy, or partner integrations assume 100 bps.
Run this to find assumptions tied to 100 bps / 1% and the default fee in the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find spots referencing the default protocol fee or old 1% value.
rg -n -S 'DEFAULT_FEE_BPS|100 bps|100bps|1% default|protocol fee|platformFeesTw' \
-g '!node_modules' -g '!dist' -g '!build' -g '!out'
# Cross-check usage around claim price collection and recipient.
rg -n -S '_collectPriceOnClaim|DEFAULT_FEE_RECIPIENT' \
-g '!node_modules' -g '!dist' -g '!build' -g '!out'Length of output: 17309
Default protocol fee halved to 50 bps — update tests assuming 100 bps
Several marketplace tests hardcode a 1% (100 bps) default fee and must be revised to reflect the new 50 bps default:
• src/test/marketplace/Offers.t.sol:364
• src/test/marketplace/DirectListings.t.sol:590
• src/test/marketplace/EnglishAuctions.t.sol:425
• src/test/marketplace/english-auctions/_payout/_payout.t.sol:267
• src/test/marketplace/direct-listings/_payout/_payout.t.sol:292
Action items:
- Adjust platformFeeBps values: for “100% total fees” use 10 000 – 50 = 9950 instead of 9900; for “99.99% total” use 9999 – 50 = 9949 instead of 9899.
- Update accompanying comments to reference 50 bps default.
- Audit any remaining docs, UI copy, deployment configs, or partner integrations for references to a 1% default fee.
🤖 Prompt for AI Agents
In contracts/prebuilts/drop/DropERC721C.sol at line 74, the default protocol fee
has been changed from 100 bps to 50 bps. You need to update all related
marketplace test files listed in the comment to reflect this change by adjusting
platformFeeBps values accordingly: use 9950 instead of 9900 for 100% total fees
and 9949 instead of 9899 for 99.99% total fees. Also, update any comments in
those tests to mention the new 50 bps default fee and audit other documentation,
UI text, deployment configs, or partner integrations for references to the old
1% fee and update them as needed.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/test/marketplace/english-auctions/collectAuctionPayout/collectAuctionPayout.t.sol (1)
298-303: Centralize the fee bps into a named constantTo avoid scattered “magic” numbers and improve clarity, define a
DEFAULT_FEE_BPSconstant at the top of the test and use it in the calculation. A quick ripgrep scan found no other raw BPS patterns based on100insrc/test, so this change will cover all occurrences.• File:
src/test/marketplace/english-auctions/collectAuctionPayout/collectAuctionPayout.t.sol
Lines: 298–303Proposed diff:
--- a/src/test/marketplace/english-auctions/collectAuctionPayout/collectAuctionPayout.t.sol +++ b/src/test/marketplace/english-auctions/collectAuctionPayout/collectAuctionPayout.t.sol @@ –10,6 +10,7 @@ contract CollectAuctionPayoutTest is Test { // ...other setup code... + uint256 private constant DEFAULT_FEE_BPS = 50; address private seller; IERC20 private erc20; Marketplace private marketplace; @@ -298,7 +299,7 @@ contract CollectAuctionPayoutTest is Test { uint256 marketplaceBal = erc20.balanceOf(address(marketplace)); - uint256 defaultFee = (marketplaceBal * 50) / 10_000; + uint256 defaultFee = (marketplaceBal * DEFAULT_FEE_BPS) / 10_000; assertEq(erc20.balanceOf(address(marketplace)), 0); assertEq(erc20.balanceOf(seller), marketplaceBal - defaultFee);
♻️ Duplicate comments (1)
src/test/marketplace/english-auctions/_payout/_payout.t.sol (1)
293-293: Same here: drop the magic number (50 bps)Mirror the change to use a named constant to keep tests DRY and future-proof.
🧹 Nitpick comments (29)
src/test/drop/drop-erc20/_collectPriceOnClaim/_collectPriceOnClaim.t.sol (2)
126-126: Replace magic number 50 bps with a named constantKeep the fee bps centralized to avoid future drift.
- uint256 defaultPlatformFeeVal = (pricePerToken * 50) / MAX_BPS; + uint256 defaultPlatformFeeVal = (pricePerToken * DEFAULT_FEE_BPS) / MAX_BPS;Add once near the top of the test:
uint256 private constant DEFAULT_FEE_BPS = 50;
219-222: Use a named bps constant for default feeAvoid repeating the literal 50 bps.
- uint256 defaultPlatformFeeVal = (pricePerToken * 50) / MAX_BPS; + uint256 defaultPlatformFeeVal = (pricePerToken * DEFAULT_FEE_BPS) / MAX_BPS;Note: In the native path, consider computing with a single totalPrice = msgValue to keep parity with ERC20 tests.
src/test/drop/drop-erc721/_collectPriceOnClaim/_collectPriceOnClaim.t.sol (2)
120-123: Drop the 50 bps magic numberIntroduce a named constant to improve maintainability.
- uint256 defaultPlatformFeeVal = (collectPrice_pricePerToken * 50) / MAX_BPS; + uint256 defaultPlatformFeeVal = (collectPrice_pricePerToken * DEFAULT_FEE_BPS) / MAX_BPS;Add once near the top of the test:
uint256 private constant DEFAULT_FEE_BPS = 50;
144-147: Same: use a named constant for default fee bpsKeeps tests consistent and change-friendly.
- uint256 defaultPlatformFeeVal = (collectPrice_pricePerToken * 50) / MAX_BPS; + uint256 defaultPlatformFeeVal = (collectPrice_pricePerToken * DEFAULT_FEE_BPS) / MAX_BPS;src/test/token/TokenERC1155.t.sol (4)
257-268: Avoid magic number (50 bps) in default fee calculationPrefer a named constant for clarity and future-proofing.
- uint256 defaultFee = ((_mintrequest.pricePerToken * _mintrequest.quantity) * 50) / MAX_BPS; + uint256 defaultFee = ((_mintrequest.pricePerToken * _mintrequest.quantity) * DEFAULT_FEE_BPS) / MAX_BPS;Declare once in the test:
uint256 private constant DEFAULT_FEE_BPS = 50;
298-309: Same: use constant instead of 50 bps literalKeeps fee logic consistent across tests.
- uint256 defaultFee = ((_mintrequest.pricePerToken * _mintrequest.quantity) * 50) / MAX_BPS; + uint256 defaultFee = ((_mintrequest.pricePerToken * _mintrequest.quantity) * DEFAULT_FEE_BPS) / MAX_BPS;
703-735: Same: refactor to a named DEFAULT_FEE_BPSReduce brittleness across different fee tests.
- uint256 defaultFee = (_mintrequest.pricePerToken * _mintrequest.quantity * 50) / 10_000; + uint256 defaultFee = (_mintrequest.pricePerToken * _mintrequest.quantity * DEFAULT_FEE_BPS) / 10_000;
765-782: Same improvement hereReplace hardcoded 50 bps with the named constant to keep things consistent.
- uint256 defaultFee = (_mintrequest.pricePerToken * _mintrequest.quantity * 50) / 10_000; + uint256 defaultFee = (_mintrequest.pricePerToken * _mintrequest.quantity * DEFAULT_FEE_BPS) / 10_000;src/test/open-edition-flat-fee/_collectPriceOnClaim/_collectPriceOnClaim.t.sol (3)
170-176: Avoid hardcoding 1 ether and magic bps; use variables/constantsUse pricePerToken and named constants for bps to reduce brittleness and duplication.
Apply this diff in this block:
- uint256 defaultFee = (1 ether * 50) / 10_000; - uint256 platformFeeVal = (1 ether * platformFeeBps) / 10_000; - uint256 primarySaleRecipientVal = 1 ether - platformFeeVal - defaultFee; + uint256 defaultFee = (pricePerToken * DEFAULT_FEE_BPS) / MAX_BPS; + uint256 platformFeeVal = (pricePerToken * platformFeeBps) / MAX_BPS; + uint256 primarySaleRecipientVal = pricePerToken - platformFeeVal - defaultFee;Add these helper constants to the test contract:
uint256 private constant DEFAULT_FEE_BPS = 50; uint256 private constant MAX_BPS = 10_000;
196-202: Repeat: remove 1 ether literals; rely on pricePerToken and named bpsSame refactor as above for consistency and maintainability.
- uint256 defaultFee = (1 ether * 50) / 10_000; - uint256 platformFeeVal = (1 ether * platformFeeBps) / 10_000; - uint256 primarySaleRecipientVal = 1 ether - platformFeeVal - defaultFee; + uint256 defaultFee = (pricePerToken * DEFAULT_FEE_BPS) / MAX_BPS; + uint256 platformFeeVal = (pricePerToken * platformFeeBps) / MAX_BPS; + uint256 primarySaleRecipientVal = pricePerToken - platformFeeVal - defaultFee;
221-227: Prefer named bps constants to magic numbersThis keeps tests aligned if DEFAULT_FEE_BPS changes again.
- uint256 defaultFee = (msgValue * 50) / 10_000; - uint256 platformFeeVal = (msgValue * platformFeeBps) / 10_000; + uint256 defaultFee = (msgValue * DEFAULT_FEE_BPS) / MAX_BPS; + uint256 platformFeeVal = (msgValue * platformFeeBps) / MAX_BPS;Add once per test contract:
uint256 private constant DEFAULT_FEE_BPS = 50; uint256 private constant MAX_BPS = 10_000;src/test/marketplace/EnglishAuctions.t.sol (10)
259-271: Replace magic numbers with named constants for default feeComputation is correct at 50 bps. To avoid repeated literals and ease future changes, use constants.
- uint256 defaultFee = (buyoutAmount * 50) / 10_000; + uint256 defaultFee = (buyoutAmount * DEFAULT_FEE_BPS) / MAX_BPS;Add once in this test contract:
uint256 private constant DEFAULT_FEE_BPS = 50; uint256 private constant MAX_BPS = 10_000;
310-318: Same suggestion: centralize bps constantsThe 50/10_000 literals are correct but brittle. Use DEFAULT_FEE_BPS and MAX_BPS as above.
- uint256 defaultFee = (buyoutAmount * 50) / 10_000; + uint256 defaultFee = (buyoutAmount * DEFAULT_FEE_BPS) / MAX_BPS;
348-355: Same: replace 50 bps literals with constantsKeeps tests self-documenting and robust.
- uint256 defaultFee = (buyoutAmount * 50) / 10_000; + uint256 defaultFee = (buyoutAmount * DEFAULT_FEE_BPS) / MAX_BPS;
395-411: Same: avoid magic numbers in defaultFee computation- uint256 defaultFee = (buyoutAmount * 50) / 10_000; + uint256 defaultFee = (buyoutAmount * DEFAULT_FEE_BPS) / MAX_BPS;
425-428: Derive 9950 programmatically from MAX_BPS and DEFAULT_FEE_BPSThe intent is MAX_BPS - DEFAULT_FEE_BPS. Compute it to avoid mismatches if either changes.
- uint128 platformFeeBps = 9950; // equal to max bps 10_000 or 100% with 50 bps default + uint128 platformFeeBps = uint128(MAX_BPS - DEFAULT_FEE_BPS); // max fee minus default feeAdd once in this test contract (if not already present):
uint256 private constant DEFAULT_FEE_BPS = 50; uint256 private constant MAX_BPS = 10_000;
1517-1526: Use constants for default fee in buyout payoutMath is correct; prefer constants for clarity.
- uint256 defaultFee = (10 ether * 50) / 10_000; + uint256 defaultFee = (10 ether * DEFAULT_FEE_BPS) / MAX_BPS;Add the same constants to this test contract as in earlier comments.
1566-1572: Use constants for default fee in post-auction payout- uint256 defaultFee = (5 ether * 50) / 10_000; + uint256 defaultFee = (5 ether * DEFAULT_FEE_BPS) / MAX_BPS;
1954-1962: Use constants for default fee in native token payout- uint256 defaultFee = (10 ether * 50) / 10_000; + uint256 defaultFee = (10 ether * DEFAULT_FEE_BPS) / MAX_BPS;Note: define DEFAULT_FEE_BPS and MAX_BPS in this test contract as well.
2301-2305: Use constants for default fee in attack POC test- uint256 defaultFee = (buyoutBidAmount * 50) / 10_000; + uint256 defaultFee = (buyoutBidAmount * DEFAULT_FEE_BPS) / MAX_BPS;Also add the same constants to this test contract.
147-149: Nit: fix typo in local variable nameMinor readability nit.
- uint256 totoalAuctions = EnglishAuctionsLogic(marketplace).totalAuctions(); - assertEq(totoalAuctions, 0); + uint256 totalAuctions = EnglishAuctionsLogic(marketplace).totalAuctions(); + assertEq(totalAuctions, 0);src/test/drop/drop-erc1155/collectPriceOnClaim/collectPriceOnClaim.t.sol (1)
155-155: Avoid magic number for default fee (50 bps); centralize to reduce future churn.These occurrences hardcode 50 bps. Prefer referencing a single constant to keep tests resilient if DEFAULT_FEE_BPS changes again.
Apply this diff within the shown lines:
- uint256 expectedDefaultPlatformFee = (collectPrice_pricePerToken * 50) / MAX_BPS; + uint256 expectedDefaultPlatformFee = (collectPrice_pricePerToken * DEFAULT_FEE_BPS_LOCAL) / MAX_BPS;Add a local constant to the test contract to avoid duplication (place near other state vars, e.g., after Line 34):
// Test-local constant to mirror contract’s configured default fee bps. uint256 private constant DEFAULT_FEE_BPS_LOCAL = 50;If available in the implementation, you can alternatively read from the contract to eliminate duplication entirely:
uint256 expectedDefaultPlatformFee = (collectPrice_pricePerToken * proxy.DEFAULT_FEE_BPS()) / MAX_BPS;Note: Only use the getter if the proxy exposes the selector for DEFAULT_FEE_BPS; otherwise stick to the test-local constant.
Also applies to: 180-180, 215-215, 239-239, 269-269, 294-294
src/test/marketplace/direct-listings/_payout/_payout.t.sol (2)
269-269: Avoid magic number for default fee (50 bps); centralize.Use a single constant or call a getter (if exposed) instead of repeating 50 inline.
Minimal change:
- uint256 defaultFee = (totalPrice * 50) / 10_000; + uint256 defaultFee = (totalPrice * DEFAULT_FEE_BPS_LOCAL) / 10_000;Add once near the top of the test contract:
uint256 private constant DEFAULT_FEE_BPS_LOCAL = 50;Or, if the marketplace/extension exposes a getter for DEFAULT_FEE_BPS and it’s routed, prefer:
uint256 defaultFee = (totalPrice * MarketplaceV3(marketplace).DEFAULT_FEE_BPS()) / 10_000;Also applies to: 338-338
292-292: Update stale comment to reflect new default fee assumption.Comment mentions “99.99% fees with 100 bps default” but code now uses 50 bps elsewhere. Consider rewording to avoid confusion, e.g., “Set platform fee very high to trigger revert when combined with default fee.”
src/test/marketplace/DirectListings.t.sol (1)
440-441: Replace repeated 50 bps default fee with a shared constant/helper.Prevent scatter of the 50 literal to ease future changes and improve readability.
Option A (least intrusive):
- uint256 defaultFee = (totalPrice * 50) / 10_000; + uint256 defaultFee = (totalPrice * DEFAULT_FEE_BPS_LOCAL) / 10_000;Declare once in the test contract:
uint256 private constant DEFAULT_FEE_BPS_LOCAL = 50;Option B (DRYest): add a small helper
function _defaultFee(uint256 total) internal pure returns (uint256) { return (total * DEFAULT_FEE_BPS_LOCAL) / 10_000; }then use:
uint256 defaultFee = _defaultFee(totalPrice);Also applies to: 486-487, 515-515, 560-561, 1522-1522, 1578-1578
src/test/tokenerc1155-BTT/mint-with-signature/mintWithSignature.t.sol (2)
537-543: Default fee updated to 50 bps — consider referencing the contract constant to avoid magic numbers.LGTM for the 50 bps change. To make tests resilient to future tweaks, prefer using the contract’s DEFAULT_FEE_BPS if exposed.
Example diff (if available on TokenERC1155/MyTokenERC1155):
- uint256 defaultFee = (totalPrice * 50) / 10_000; + uint256 defaultFee = (totalPrice * tokenContract.DEFAULT_FEE_BPS()) / 10_000;Optionally, add a small helper in this test contract to DRY:
function _defaultFee(uint256 totalPrice) internal view returns (uint256) { return (totalPrice * tokenContract.DEFAULT_FEE_BPS()) / 10_000; }
643-649: ERC20 path aligned to 50 bps — good.Change is consistent with DEFAULT_FEE_BPS = 50. Consider the same constant-based refactor as above to reduce test churn.
src/test/marketplace/Offers.t.sol (2)
208-221: Default fee moved to 50 bps — prefer sourcing from a single constant.Implementation looks correct. If the default fee bps is exposed (e.g., by RoyaltyPaymentsLogic or MarketplaceV3), consider referencing it to avoid repeating
50.
365-367: Platform fee bps adjusted to maintain total fee sum — consider deriving from denominator minus default fee.Using 9950 keeps platform + default fee at 10_000 bps (100%) and — with royalties present — guarantees revert. To improve maintainability, derive this from the denominator and the default fee constant if accessible.
For example (if a constant exists):
uint128 platformFeeBps = uint128(10_000 - <DEFAULT_FEE_BPS>());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/test/drop/drop-erc1155/collectPriceOnClaim/collectPriceOnClaim.t.sol(6 hunks)src/test/drop/drop-erc20/_collectPriceOnClaim/_collectPriceOnClaim.t.sol(4 hunks)src/test/drop/drop-erc721/_collectPriceOnClaim/_collectPriceOnClaim.t.sol(2 hunks)src/test/marketplace/DirectListings.t.sol(7 hunks)src/test/marketplace/EnglishAuctions.t.sol(9 hunks)src/test/marketplace/Offers.t.sol(6 hunks)src/test/marketplace/direct-listings/_payout/_payout.t.sol(2 hunks)src/test/marketplace/english-auctions/_payout/_payout.t.sol(2 hunks)src/test/marketplace/english-auctions/collectAuctionPayout/collectAuctionPayout.t.sol(1 hunks)src/test/open-edition-flat-fee/_collectPriceOnClaim/_collectPriceOnClaim.t.sol(4 hunks)src/test/smart-wallet/utils/AABenchmarkArtifacts.sol(1 hunks)src/test/token/TokenERC1155.t.sol(4 hunks)src/test/tokenerc1155-BTT/mint-with-signature/mintWithSignature.t.sol(3 hunks)src/test/utils/BaseTest.sol(0 hunks)
💤 Files with no reviewable changes (1)
- src/test/utils/BaseTest.sol
✅ Files skipped from review due to trivial changes (1)
- src/test/smart-wallet/utils/AABenchmarkArtifacts.sol
🔇 Additional comments (8)
src/test/open-edition-flat-fee/_collectPriceOnClaim/_collectPriceOnClaim.t.sol (1)
141-147: LGTM: default fee math correctly updated to 50 bpsThe computation and downstream assertions align with the halved DEFAULT_FEE_BPS.
src/test/drop/drop-erc1155/collectPriceOnClaim/collectPriceOnClaim.t.sol (1)
155-155: No changes needed: using a local 50 bps constant is correctDropERC1155 (and the other Drop contracts) declare
DEFAULT_FEE_BPSas a private constant with no external getter, so referencing50directly in your test is appropriate. The math/truncation remains correct with 50 bps, and balances will still sum as expected.src/test/marketplace/DirectListings.t.sol (1)
591-591: Good adjustment to keep total fees at 100% for the revert test.Setting platformFeeBps to 9950 with a 50 bps default keeps the sum at 10_000 bps, preserving the original invariant for the “fees exceed the price” revert case.
src/test/tokenerc1155-BTT/mint-with-signature/mintWithSignature.t.sol (1)
825-831: Flat platform fee path aligned to 50 bps — good.The default fee computation matches the new basis points and the subsequent assertions are consistent.
src/test/marketplace/Offers.t.sol (4)
255-263: ERC2981 path updated to 50 bps — correct.Assertions and net proceeds math align with the halved default fee.
288-297: Default ERC2981 token case updated to 50 bps — correct.Consistent with the global change. No further issues.
332-350: “Distribute all fees” test updated to 50 bps — correct.Math and assertions reflect the new default correctly.
806-812: Accept offer test updated to 50 bps — correct.Net transfer checks and balances align with updated default fee.
| uint256 defaultPlatformFeeVal = (pricePerToken * 50) / MAX_BPS; | ||
| uint256 platformFeeVal = (pricePerToken * platformFeeBps) / MAX_BPS; | ||
| uint256 primarySaleRecipientVal = 1 ether - platformFeeVal - defaultPlatformFeeVal; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a named bps constant and avoid hardcoded 1 ether in proceeds calc
- Drop magic number 50 bps.
- Use pricePerToken (or total price) instead of 1 ether to avoid fragile coupling to current test data.
- uint256 defaultPlatformFeeVal = (pricePerToken * 50) / MAX_BPS;
+ uint256 defaultPlatformFeeVal = (pricePerToken * DEFAULT_FEE_BPS) / MAX_BPS;
uint256 platformFeeVal = (pricePerToken * platformFeeBps) / MAX_BPS;
- uint256 primarySaleRecipientVal = 1 ether - platformFeeVal - defaultPlatformFeeVal;
+ uint256 primarySaleRecipientVal = pricePerToken - platformFeeVal - defaultPlatformFeeVal;If you prefer a single source of truth, compute once:
// outside changed lines
uint256 totalPrice = pricePerToken; // ERC20 branch
uint256 defaultPlatformFeeVal = (totalPrice * DEFAULT_FEE_BPS) / MAX_BPS;
uint256 platformFeeVal = (totalPrice * platformFeeBps) / MAX_BPS;
uint256 primarySaleRecipientVal = totalPrice - platformFeeVal - defaultPlatformFeeVal;🤖 Prompt for AI Agents
In src/test/drop/drop-erc20/_collectPriceOnClaim/_collectPriceOnClaim.t.sol
around lines 160 to 163, replace the hardcoded 50 bps with a named constant like
DEFAULT_FEE_BPS and avoid using the hardcoded 1 ether in the proceeds
calculation. Instead, define a single totalPrice variable set to pricePerToken
and use it to calculate defaultPlatformFeeVal, platformFeeVal, and
primarySaleRecipientVal to ensure consistency and reduce fragile coupling to
test data.
| uint256 defaultPlatformFeeVal = (pricePerToken * 50) / MAX_BPS; | ||
| uint256 platformFeeVal = (pricePerToken * platformFeeBps) / MAX_BPS; | ||
| uint256 primarySaleRecipientVal = 1 ether - platformFeeVal - defaultPlatformFeeVal; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same improvement: constant bps and remove 1 ether literal
Mirror the ERC20 proceeds calc fix to avoid hidden assumptions.
- uint256 defaultPlatformFeeVal = (pricePerToken * 50) / MAX_BPS;
+ uint256 defaultPlatformFeeVal = (pricePerToken * DEFAULT_FEE_BPS) / MAX_BPS;
uint256 platformFeeVal = (pricePerToken * platformFeeBps) / MAX_BPS;
- uint256 primarySaleRecipientVal = 1 ether - platformFeeVal - defaultPlatformFeeVal;
+ uint256 primarySaleRecipientVal = pricePerToken - platformFeeVal - defaultPlatformFeeVal;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 defaultPlatformFeeVal = (pricePerToken * 50) / MAX_BPS; | |
| uint256 platformFeeVal = (pricePerToken * platformFeeBps) / MAX_BPS; | |
| uint256 primarySaleRecipientVal = 1 ether - platformFeeVal - defaultPlatformFeeVal; | |
| uint256 defaultPlatformFeeVal = (pricePerToken * DEFAULT_FEE_BPS) / MAX_BPS; | |
| uint256 platformFeeVal = (pricePerToken * platformFeeBps) / MAX_BPS; | |
| uint256 primarySaleRecipientVal = pricePerToken - platformFeeVal - defaultPlatformFeeVal; |
🤖 Prompt for AI Agents
In src/test/drop/drop-erc20/_collectPriceOnClaim/_collectPriceOnClaim.t.sol
around lines 190 to 193, replace the literal 1 ether with a named constant
representing the total amount to avoid hidden assumptions. Also, define a
constant for MAX_BPS if not already defined, and use these constants
consistently in the calculation of defaultPlatformFeeVal, platformFeeVal, and
primarySaleRecipientVal to improve clarity and maintainability.
| @@ -240,7 +240,7 @@ contract EnglishAuctionsPayoutTest is BaseTest, IExtension { | |||
| uint256 platformFees = (totalPrice * platformFeeBps) / 10_000; | |||
|
|
|||
| { | |||
| uint256 defaultFee = (totalPrice * 100) / 10_000; | |||
| uint256 defaultFee = (totalPrice * 50) / 10_000; | |||
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Avoid magic numbers for default fee bps
Use a named constant instead of hardcoding 50 to reduce brittleness and ease future updates.
Apply within this test:
- uint256 defaultFee = (totalPrice * 50) / 10_000;
+ uint256 defaultFee = (totalPrice * DEFAULT_FEE_BPS) / 10_000;Add near the top of the contract:
uint256 private constant DEFAULT_FEE_BPS = 50;Optional: scan for any leftover 100 bps references still tied to default fee:
🏁 Script executed:
#!/bin/bash
# Find potentially stale default-fee computations still using 100 bps in tests
rg -n --no-heading -e '\(\s*[^)]*\s*\*\s*100\s*\)\s*/\s*(MAX_BPS|10_000)' -e 'DEFAULT_FEE_BPS\s*=\s*100' src/testLength of output: 115
Define and use DEFAULT_FEE_BPS instead of the raw 50
To eliminate the magic number and make future fee-bps updates easier, introduce a constant and reference it in your test:
• At the top of src/test/marketplace/english-auctions/_payout/_payout.t.sol, add:
uint256 private constant DEFAULT_FEE_BPS = 50;• Replace both occurrences of the hardcoded 50 in fee calculations:
- uint256 defaultFee = (totalPrice * 50) / 10_000;
+ uint256 defaultFee = (totalPrice * DEFAULT_FEE_BPS) / 10_000;I ran a grep across src/test for any other raw “100” or “50” fee-bps patterns and found no remaining stale references. Applying this change will keep the test DRY and resilient to future fee-bps adjustments.
🤖 Prompt for AI Agents
In src/test/marketplace/english-auctions/_payout/_payout.t.sol at line 243,
replace the hardcoded fee basis points value 50 with a constant named
DEFAULT_FEE_BPS. Define this constant as a private uint256 at the top of the
file with the value 50. Then update all fee calculations in the file to use
DEFAULT_FEE_BPS instead of the raw number 50 to avoid magic numbers and improve
maintainability.
Summary by CodeRabbit