feat: Add ERC20 submission fee system to RepositoryRegistry #107
Conversation
…in controls, tests)
WalkthroughAdds ERC20-based submission fees to RepositoryRegistry: new token, fee wallet, and fee amount state; fee transfer on submit; updated events and constructor; owner-setters for fee and wallet. Tests introduce a mock token, minting, approvals, fee assertions, and failure on insufficient allowance. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Maintainer
participant Registry as RepositoryRegistry
participant Token as ERC20 Token
participant Wallet as Fee Wallet
rect rgba(230,245,255,0.6)
Maintainer->>Registry: submitRepository(...)
note right of Registry: Validate inputs
Registry->>Token: safeTransferFrom(Maintainer, Wallet, submissionFee)
Token-->>Registry: transfer success/fail
alt Fee transfer succeeds
Registry-->>Wallet: (Fee credited via Token)
Registry-->>Maintainer: emit RepositorySubmitted(id, maintainer, feePaid)
else Fee transfer fails
Registry-->>Maintainer: revert (insufficient allowance/balance)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 (8)
test/RepositoryRegistry.ts (5)
203-235: Also assert payer balance delta and registry never holds feesStrengthen this fee test by checking:
- maintainer1 balance decreases by
submissionFee- registry token balance stays 0 (fees go straight to
feeWallet)- const feeWalletBefore = await mockDEVToken.read.balanceOf([feeWallet.account.address]); + const feeWalletBefore = await mockDEVToken.read.balanceOf([feeWallet.account.address]); + const maintainerBefore = await mockDEVToken.read.balanceOf([maintainer1.account.address]); + const registryBefore = await mockDEVToken.read.balanceOf([repositoryRegistry.address]); @@ - const feeWalletAfter = await mockDEVToken.read.balanceOf([feeWallet.account.address]); + const feeWalletAfter = await mockDEVToken.read.balanceOf([feeWallet.account.address]); + const maintainerAfter = await mockDEVToken.read.balanceOf([maintainer1.account.address]); + const registryAfter = await mockDEVToken.read.balanceOf([repositoryRegistry.address]); @@ expect(repo.tags).to.deep.equal(tags); expect(feeWalletAfter - feeWalletBefore).to.equal(submissionFee); + expect(maintainerBefore - maintainerAfter).to.equal(submissionFee); + expect(registryBefore).to.equal(0n); + expect(registryAfter).to.equal(0n);
255-273: Decode and assert RepositorySubmitted event args (id, maintainer, feePaid)Current check only asserts “has logs”. Decode the specific event and assert its parameters, including
feePaid.- const receipt = await publicClient.waitForTransactionReceipt({ hash }); - - expect(receipt.logs).to.have.lengthOf.at.least(1); - expect(receipt.status).to.equal("success"); + const receipt = await publicClient.waitForTransactionReceipt({ hash }); + expect(receipt.status).to.equal("success"); + + // Decode RepositorySubmitted(id, maintainer, feePaid) + const { decodeEventLog, parseAbiItem } = await import("viem"); + const evt = parseAbiItem( + 'event RepositorySubmitted(uint256 indexed id, address indexed maintainer, uint256 feePaid)' + ); + const log = receipt.logs.find(l => l.address.toLowerCase() === repositoryRegistry.address.toLowerCase()); + expect(log).to.not.be.undefined; + const decoded = decodeEventLog({ ...log!, abi: [evt] }); + expect(decoded.eventName).to.equal("RepositorySubmitted"); + // id should be 1 on first submission + expect(decoded.args.id).to.equal(1n); + expect(decoded.args.maintainer.toLowerCase()).to.equal(maintainer1.account.address.toLowerCase()); + // feePaid should equal the configured submissionFee + // (cannot import fixture variables here, so just assert > 0n or thread it through if accessible) + expect(decoded.args.feePaid > 0n).to.be.true;
236-253: Tighter assertion on allowance failurePrefer asserting a specific revert (custom error or message) to avoid false positives. If your MockDEVToken uses OZ v5, it likely reverts with
ERC20InsufficientAllowance(custom error).If it’s OZ v5, update to:
- ).to.be.rejected; + ).to.be.rejectedWith('ERC20InsufficientAllowance');Else, adapt to your mock’s actual revert. If you share
MockDEVToken.sol, I can tailor the exact assertion.
48-57: Scope approvals to the exact fee to reflect real usageApproving
mintAmountis over‑permissive. Approve justsubmissionFeeinside tests that submit, and skip approval entirely in tests that expect allowance failure.- await mockDEVToken.write.approve([ - repositoryRegistry.address, - mintAmount, - ], { account: maintainer1.account }); + await mockDEVToken.write.approve([ + repositoryRegistry.address, + submissionFee, + ], { account: maintainer1.account });Alternatively, create a second fixture without approvals for the “insufficient allowance” test to avoid mutation in the test body.
12-21: Process mismatch: PR objective references withdrawVote, but tests/changes add ERC20 feesLinked issue says “withdrawVote(...)” while this PR adds submission fees. Confirm linkage or update the issue reference so reviewers aren’t misled.
Also applies to: 33-36
contracts/RepositoryRegistry.sol (3)
104-114: Admin: permit disabling fees and add guardrails
- Setting fee to 0 should intentionally disable charging (aligned with the refactor above).
- Consider upper‑bound sanity checks to avoid accidental large fees (optional).
54-61: Consistent not‑found/error messaging across functions
updateRepositoryreverts with “Not owner” for missing IDs, whiledeactivateRepositoryuses “Repository does not exist”. Consider aligning for clearer DX (e.g., first check existence, then ownership).
70-99: Minor: gas/style
repoCounter += 1;can be++repoCounter;and cached asuint256 id = ++repoCounter;to avoid repeated map indexing when emitting or extending logic.tags: tagscopies calldata array into storage; this is fine in 0.8.x, just noting the cost. No action required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/RepositoryRegistry.sol(4 hunks)test/RepositoryRegistry.ts(4 hunks)
🔇 Additional comments (2)
contracts/RepositoryRegistry.sol (2)
4-9: Dependency/version sanity check (OZ v5 vs v4 pathing)
ReentrancyGuardlives underutilsin OZ v5, undersecurityin v4. Ensure the repo uses OZ v5 to match this import; otherwise switch the path.I can auto‑verify with a quick script if you want.
32-38: Restore legacy RepositorySubmitted signature and emit a separate fee eventChanging RepositorySubmitted to include feePaid alters the event signature (topic) and will break existing indexers/consumers. Restore/keep the original two‑arg event (id, maintainer) and emit it alongside a new fee event (e.g., SubmissionFeePaid(uint256 id, address payer, uint256 amount)) from submitRepository so consumers keep working.
File: contracts/RepositoryRegistry.sol — current events:
// Events event RepositorySubmitted(uint256 indexed id, address indexed maintainer, uint256 feePaid); event RepositoryUpdated(uint256 indexed id, address indexed maintainer); event RepositoryDeactivated(uint256 indexed id); event SubmissionFeeUpdated(uint256 oldFee, uint256 newFee); event FeeWalletUpdated(address oldWallet, address newWallet);
| // Require fee payment | ||
| require(submissionFee > 0, "Submission fee not set"); | ||
| token.safeTransferFrom(msg.sender, feeWallet, submissionFee); | ||
|
|
There was a problem hiding this comment.
Allow zero fee to disable charging; move external transfer after state updates (CEI)
- Requiring
submissionFee > 0prevents disabling fees, contradicting “Backward compatibility maintained.” - Perform the ERC20 transfer after storage writes to follow checks‑effects‑interactions.
nonReentrantremains a good guard; a revert still rolls back state.
- // Require fee payment
- require(submissionFee > 0, "Submission fee not set");
- token.safeTransferFrom(msg.sender, feeWallet, submissionFee);
+ // Effects first, interactions later (fee may be zero to disable charging)
@@
- // Emit event
- emit RepositorySubmitted(repoCounter, msg.sender, submissionFee);
+ // Interactions (charge fee if enabled)
+ uint256 fee = submissionFee;
+ if (fee > 0) {
+ token.safeTransferFrom(msg.sender, feeWallet, fee);
+ }
+ // Emit event
+ emit RepositorySubmitted(repoCounter, msg.sender, fee);Also applies to: 100-103
🤖 Prompt for AI Agents
In contracts/RepositoryRegistry.sol around lines 81-84 (and similarly lines
100-103), the code currently requires submissionFee > 0 and performs the ERC20
transfer before updating storage; remove the require so a zero submissionFee is
allowed (disabling charging), and move the token.safeTransferFrom call to after
the state changes to follow Checks-Effects-Interactions (you can keep
nonReentrant); ensure you still call safeTransferFrom only when submissionFee >
0 to avoid unnecessary external calls when fee is zero.
This pull request closes #82 by implementing a submission fee system for the
RepositoryRegistrycontract:This enables protocol-level monetization and anti-spam for repository submissions.
Summary by CodeRabbit