feat(backend): remove rejected distributions #87
Conversation
✅ Deploy Preview for web3-sponsors canceled.
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds QueueHandler.removeRejectedDistribution to dequeue the front distribution when DistributionVerifier marks it rejected; processQueuePair now only accepts approved distributions. Tests and gas report updated to exercise and report the new removal path. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant QH as QueueHandler
participant DQ as DistributionQueue
participant DV as DistributionVerifier
Caller->>QH: removeRejectedDistribution()
QH->>DQ: getLength()
alt length == 0
QH-->>Caller: revert (empty queue)
else length > 0
QH->>DQ: queueNumberFront()
QH->>DV: isDistributionRejected(frontNumber)
alt rejected == true
QH->>DQ: dequeue()
QH-->>Caller: return (success)
else rejected == false
QH-->>Caller: revert ("Only approved distributions can be processed" / not rejected)
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
elimu.ai's mission is to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ 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. 🪧 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/Issue comments)Type 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: 0
🧹 Nitpick comments (4)
backend/contracts/QueueHandler.sol (3)
83-95: Consider access control: gate removal to DAO operators or document permissionless intentRight now anyone can call this and mutate queue state. If the policy is that only DAO operators should manage queue hygiene (like
processQueuePair), add a roles check; otherwise add a NatSpec note that this is intentionally permissionless. Keeping the queue clean supports timely disbursements aligned with elimu.ai's mission: "elimu.ai's mission is to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months."Apply if you prefer gating:
- function removeRejectedDistribution() public { + function removeRejectedDistribution() external { + require(roles.isDaoOperator(msg.sender), "Only DAO operators can remove rejected distributions");
89-95: Emit an event when a rejected distribution is dequeuedAdds traceability for off-chain indexers and ops dashboards.
Apply:
- // Remove the distribution from the queue - distributionQueue.dequeue(); + // Remove the distribution from the queue + emit RejectedDistributionRemoved(distributionQueueNumber); + distributionQueue.dequeue();Add alongside existing events (near Lines 17–20):
event RejectedDistributionRemoved(uint24 distributionQueueNumber);
85-92: Prefer custom errors over string requires for gas and consistencyYou already use
DistributionRejectedError; mirror that pattern here.Apply:
- require(distributionQueue.getLength() > 0, "The distribution queue cannot be empty"); + if (distributionQueue.getLength() == 0) revert DistributionQueueEmpty(); @@ - require(isDistributionRejected, "Only rejected distributions can be removed from the queue"); + if (!isDistributionRejected) revert DistributionNotRejected();Add error declarations near Line 21:
error DistributionQueueEmpty(); error DistributionNotRejected();backend/test/QueueHandler.ts (1)
147-162: Add negative-path tests to lock behavior
- Empty queue → revert with "The distribution queue cannot be empty".
- Front not rejected (unverified or approved) → revert with the current message.
- If you add DAO-operator gating as suggested, include a test that non-operators are reverted.
You can extend this block with:
await expect(queueHandler.removeRejectedDistribution()) .to.be.rejectedWith("The distribution queue cannot be empty"); await distributionQueue.addDistribution("HIN", "abc"); await expect(queueHandler.removeRejectedDistribution()) .to.be.rejectedWith("Only rejected distributions can be removed from the queue");
📜 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 (3)
backend/contracts/QueueHandler.sol(1 hunks)backend/hardhat-gas-report.md(5 hunks)backend/test/QueueHandler.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). (3)
- GitHub Check: build (windows-latest, 20.x)
- GitHub Check: build (windows-latest, 18.x)
- GitHub Check: build (ubuntu-latest, 18.x)
🔇 Additional comments (3)
backend/test/QueueHandler.ts (1)
147-162: Good positive-path test for removalThe test clearly proves a rejected front item is removed. Setup is minimal and reuses the fixture well.
backend/hardhat-gas-report.md (2)
44-45: Gas report includes new method entry — looks rightNew
removeRejectedDistributionline present; keep this synced with test coverage going forward.
12-12: Verify report regeneration came from a clean runNumbers for add/verify/update and deployments shifted. Please confirm this file was regenerated from the same commit (e.g.,
HARDHAT_NETWORK=hardhat npm test -- --report-gas) to avoid drift across environments.Also applies to: 26-26, 58-61, 78-81
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/hardhat-gas-report.md (1)
42-45: Gas for new removal path looks healthy; add CI threshold to guard regressionsprocessQueuePair ~105.9k and removeRejectedDistribution ~54.5k align with expectations. Consider adding per-function gas ceilings in CI to prevent drift. This supports reliable payouts at scale, in line with elimu.ai's mission: elimu.ai's mission is to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
backend/contracts/QueueHandler.sol (2)
54-59: Approval-only processing: good; consider small gas/read orderingLGTM enforcing approval before processing. Optional: check sponsorshipQueue.getLength() before the verifier call to save an external read when sponsorships are empty. Also consider custom errors to shave gas.
72-74: Emit an event for successful pairingAdd an event for observability/auditing.
Apply this diff:
- // Emit event - // TODO + // Emit event + emit QueuePairProcessed(distributionQueueNumber);And declare the event (outside this hunk):
event QueuePairProcessed(uint24 distributionQueueNumber);
📜 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 ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
backend/contracts/QueueHandler.sol(2 hunks)backend/hardhat-gas-report.md(5 hunks)backend/package.json(1 hunks)backend/test/QueueHandler.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/test/QueueHandler.ts
⏰ 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). (3)
- GitHub Check: build (windows-latest, 18.x)
- GitHub Check: build (windows-latest, 20.x)
- GitHub Check: build (ubuntu-latest, 18.x)
🔇 Additional comments (2)
backend/hardhat-gas-report.md (2)
26-26: Slight verifier cost uptick; confirm root causeverifyDistribution avg rose (~63,421). Likely due to new status path usage—please confirm via test trace to ensure no unintended storage touches.
78-81: Deployments gas bump: still within budget?QueueHandler avg ~971k and SponsorshipQueue ~1.106M are <4% of block limit. Validate against your deployment budget matrix and historical baselines.
backend/contracts/QueueHandler.sol
Outdated
| /// @notice Remove rejected distribution from the distribution queue | ||
| function removeRejectedDistribution() public { | ||
| // Verify that the queue of distributions is not empty | ||
| require(distributionQueue.getLength() > 0, "The distribution queue cannot be empty"); | ||
|
|
||
| // Verify that the next distribution in the queue has been rejected | ||
| uint24 distributionQueueNumber = distributionQueue.queueNumberFront(); | ||
| bool isDistributionRejected = distributionVerifier.isDistributionRejected(distributionQueueNumber); | ||
| require(isDistributionRejected, "Only rejected distributions can be removed from the queue"); | ||
|
|
||
| // Remove the distribution from the queue | ||
| distributionQueue.dequeue(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
removeRejectedDistribution: clarify permissions + emit event
If permissionless cleanup is intended, great—document it. If not, gate with roles. Emit an event and prefer external visibility.
Apply this diff:
- function removeRejectedDistribution() public {
+ function removeRejectedDistribution() external {
- // Verify that the queue of distributions is not empty
+ // Verify that the queue of distributions is not empty
require(distributionQueue.getLength() > 0, "The distribution queue cannot be empty");
- // Verify that the next distribution in the queue has been rejected
+ // Verify that the next distribution in the queue has been rejected
uint24 distributionQueueNumber = distributionQueue.queueNumberFront();
bool isDistributionRejected = distributionVerifier.isDistributionRejected(distributionQueueNumber);
require(isDistributionRejected, "Only rejected distributions can be removed from the queue");
- // Remove the distribution from the queue
+ // Remove the distribution from the queue
distributionQueue.dequeue();
+ emit RejectedDistributionRemoved(distributionQueueNumber);
}And either add role gating:
- function removeRejectedDistribution() external {
+ function removeRejectedDistribution() external {
+ require(roles.isDaoOperator(msg.sender), "Only DAO operators can remove rejected distributions");Declare the event (outside this hunk):
event RejectedDistributionRemoved(uint24 distributionQueueNumber);🏁 Script executed:
#!/bin/bash
set -e
# Locate the QueueHandler.sol file
file=$(find . -type f -name QueueHandler.sol | head -n1)
echo "File path: $file"
echo "==== Top of file (1-200) ===="
sed -n '1,200p' "$file"
echo "==== Events in file ===="
grep -R "event " -n "$file"
echo "==== Function definitions ===="
grep -R "function " -n "$file"Length of output: 4927
Clarify removeRejectedDistribution permissions and emit event
- Change visibility from
publictoexternal. - Either document permissionless design or restrict callers via
require(roles.isDaoOperator(msg.sender), "Only DAO operators can remove rejected distributions"); - Emit an event after dequeue and declare it:
- function removeRejectedDistribution() public {
+ function removeRejectedDistribution() external {
+ require(roles.isDaoOperator(msg.sender), "Only DAO operators can remove rejected distributions");
// Verify that the queue of distributions is not empty
require(distributionQueue.getLength() > 0, "The distribution queue cannot be empty");
// Verify that the next distribution in the queue has been rejected
uint24 distributionQueueNumber = distributionQueue.queueNumberFront();
bool isDistributionRejected = distributionVerifier.isDistributionRejected(distributionQueueNumber);
require(isDistributionRejected, "Only rejected distributions can be removed from the queue");
// Remove the distribution from the queue
distributionQueue.dequeue();
+ emit RejectedDistributionRemoved(distributionQueueNumber);
}Declare the event (outside contract functions):
event RejectedDistributionRemoved(uint24 distributionQueueNumber);elimu.ai’s mission is to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
🤖 Prompt for AI Agents
In backend/contracts/QueueHandler.sol around lines 76 to 88, change
removeRejectedDistribution visibility from public to external, decide and
enforce caller permissions (either document as permissionless or add a require
like require(roles.isDaoOperator(msg.sender), "Only DAO operators can remove
rejected distributions");), and after distributionQueue.dequeue() emit an event
indicating which distribution was removed; also declare the event outside
functions as event RejectedDistributionRemoved(uint24 distributionQueueNumber);
so the emitted event uses the distributionQueueNumber variable captured before
dequeue.
Resolves #86
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores