Function paginations #11
Conversation
WalkthroughAdds pagination (offset, limit, totalCount) to many public getters across Organisation, OrganisationFactory, and TreeNft; records/promotes/removes owner/member relationships in OrganisationFactory; synchronizes verification visibility and optimized removal in TreeNft; updates tests and CI formatting behavior; removes two public variables from the deploy script. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant Org as Organisation
participant Factory as OrganisationFactory
Admin->>Org: makeOwner(newOwner)
activate Org
Note right of Org: append newOwner to owners
Org->>Factory: promoteToOwner(newOwner)
activate Factory
Note right of Factory: add org to s_userToOrganisationsAsOwner\nremove from s_userToOrganisationsAsMember if present
Factory-->>Org: ack
Org-->>Admin: success
deactivate Factory
deactivate Org
sequenceDiagram
actor Client
participant Factory as OrganisationFactory
Client->>Factory: getMyOrganisationsAsOwner(offset,limit)
activate Factory
Note right of Factory: totalCount = len(s_userToOrganisationsAsOwner[user])\nslice = items[offset:offset+limit]
Factory-->>Client: (OrganisationDetails[], totalCount)
deactivate Factory
sequenceDiagram
actor Verifier
participant Tree as TreeNft
Verifier->>Tree: getTreeNftVerifiersPaginated(tokenId,offset,limit)
activate Tree
Note right of Tree: compute visible verifications\nreturn slice and counts
Tree-->>Verifier: (TreeNftVerification[], totalCount, visibleCount)
deactivate Tree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
IronJam11
force-pushed
the
functionPaginations
branch
from
d67d6cc to
286187d
Compare
…s with chnage in tests
IronJam11
force-pushed
the
functionPaginations
branch
from
286187d to
4af74f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OrganisationFactory.sol (1)
223-236: Bug: removeOrganisation doesn’t flip isActive or clear lookup.s_isOrganisation is never set to false, and the Organisation lookup isn’t cleared. After removal, getOrganisationInfo will still succeed and “isActive” in details will remain true. Fix as below.
function removeOrganisation(address _organisationAddress) external onlyOwner { // ... - if (s_isOrganisation[_organisationAddress] == false) { + if (s_isOrganisation[_organisationAddress] == false) { revert OrganisationDoesNotExist(); } + // Mark inactive and clear lookup before list removal + s_isOrganisation[_organisationAddress] = false; + delete s_organisationAddressToOrganisation[_organisationAddress]; for (uint256 i = 0; i < s_allOrganisations.length; i++) { if (s_allOrganisations[i] == _organisationAddress) { s_allOrganisations[i] = s_allOrganisations[s_allOrganisations.length - 1]; s_allOrganisations.pop(); break; } } }Also consider emitting an event and optionally pruning the org from role-indexed arrays (may be expensive; soft-deactivation via isActive=false is acceptable if documented).
🧹 Nitpick comments (18)
src/TreeNft.sol (2)
130-145: Cap page size and guard offset+limit overflow in getAllNFTs.Limit is unchecked and offset+limit can overflow before clamping, risking OOG in contract-to-contract calls. Align with getRecentTreesPaginated’s cap and add an overflow-safe end calculation.
Apply these changes:
@@ - function getAllNFTs(uint256 offset, uint256 limit) public view returns (Tree[] memory trees, uint256 totalCount) { + function getAllNFTs(uint256 offset, uint256 limit) public view returns (Tree[] memory trees, uint256 totalCount) { totalCount = s_treeTokenCounter; - if (offset >= totalCount) { + if (offset >= totalCount) { return (new Tree[](0), totalCount); } - uint256 end = offset + limit; - if (end > totalCount) { - end = totalCount; - } + if (limit == 0) { + return (new Tree[](0), totalCount); + } + // Optional: reuse same cap as other endpoints for consistency + if (limit > 50) revert PaginationLimitExceeded(); + uint256 end = offset + limit; + // overflow-safe clamp + if (end < offset || end > totalCount) { + end = totalCount; + } uint256 resultLength = end - offset; trees = new Tree[](resultLength); for (uint256 i = 0; i < resultLength; i++) { trees[i] = s_tokenIDtoTree[offset + i]; } return (trees, totalCount); }
418-450: Avoid full-array copies; add overflow guard and page cap; clarify numberOfTrees semantics.
- TreeNftVerification[] memory userVerifications copies the entire storage array. Use a storage reference to avoid O(n) copy per call.
- Guard offset+limit overflow and cap limit like other endpoints.
- numberOfTrees uses planterToken.balanceOf(userAddress). That returns the user’s current balance for the verifier’s token, not the per-verification tree count. If that’s intended, add a NatSpec note; otherwise return the per-verification amount.
Apply:
- function getUserVerifierTokenDetails(address userAddress, uint256 offset, uint256 limit) + function getUserVerifierTokenDetails(address userAddress, uint256 offset, uint256 limit) public view returns (VerificationDetails[] memory verifierTokenDetails, uint256 totalCount) { - // This function returns the verifier token details of the user with pagination - - TreeNftVerification[] memory userVerifications = s_userToVerifications[userAddress]; + // Returns the verifier token details of the user with pagination. + // numberOfTrees reflects the user's current balance for each verifier's PlanterToken. + TreeNftVerification[] storage userVerifications = s_userToVerifications[userAddress]; totalCount = userVerifications.length; - if (offset >= totalCount) { + if (offset >= totalCount) { return (new VerificationDetails[](0), totalCount); } - uint256 end = offset + limit; - if (end > totalCount) { - end = totalCount; - } + if (limit == 0) { + return (new VerificationDetails[](0), totalCount); + } + // Optional: unify cap across endpoints + if (limit > 50) revert PaginationLimitExceeded(); + uint256 end = offset + limit; + if (end < offset || end > totalCount) { + end = totalCount; + } uint256 resultLength = end - offset; verifierTokenDetails = new VerificationDetails[](resultLength); for (uint256 i = 0; i < resultLength; i++) { uint256 verificationIndex = offset + i; PlanterToken planterToken = PlanterToken(userVerifications[verificationIndex].verifierPlanterTokenAddress); verifierTokenDetails[i] = VerificationDetails({ verifier: userVerifications[verificationIndex].verifier, timestamp: userVerifications[verificationIndex].timestamp, proofHashes: userVerifications[verificationIndex].proofHashes, description: userVerifications[verificationIndex].description, isHidden: userVerifications[verificationIndex].isHidden, numberOfTrees: planterToken.balanceOf(userAddress), verifierPlanterTokenAddress: userVerifications[verificationIndex].verifierPlanterTokenAddress }); } return (verifierTokenDetails, totalCount); }Optionally, add a brief NatSpec above the function clarifying the meaning of numberOfTrees to avoid confusion.
test/TreeNft.t.sol (1)
135-139: Strengthen pagination assertions and edge coverage.Good migration to paginated getters. Two improvements:
- Also assert totalCount/visibleCount where available to ensure the count semantics remain correct under slicing and after mutations (e.g., after removeVerification).
- Add tests for overflow and cap behavior: offset near type max and limit > cap should revert with PaginationLimitExceeded once caps are enforced consistently.
Example tweaks:
- Capture (,,visibleCount) from getTreeNftVerifiersPaginated and assert visibleCount matches expected after hiding.
- For getUserVerifierTokenDetails, assert totalCount across pages equals the sum of returned lengths, and that offset >= totalCount yields empty arrays.
Would you like me to push a follow-up test commit covering these cases once the caps/overflow guards are added?
Also applies to: 162-172, 215-221, 308-312, 332-336, 429-446, 474-486, 515-535, 537-543, 545-572, 574-599, 600-631
src/OrganisationFactory.sol (11)
38-38: Creation path OK; verify returned organisationId semantics.Pushing creator into owner index is correct. However, createOrganisation still returns organisationId that is always 0. Either implement an incrementing ID or drop it from the return to avoid misleading callers.
45-65: Pagination slice looks correct.Bounds checks and slicing are fine; zero limit yields empty set. Consider extracting a small internal helper to DRY with other paginated address getters.
67-73: Avoid external self-call for gas.return this.getUserOrganisations(...) incurs an external call. Prefer an internal/private helper to slice once and reuse.
75-94: All-organisations pagination LGTM.Slice math and early-exit are correct. Same note as above about extracting a helper.
102-114: Minor: redundant and missing validations.
- msg.sender can’t be address(0); that check is redundant after s_isOrganisation[msg.sender].
- Consider require _member != address(0) as a defensive check.
116-135: promoteToOwner indexing can drift without invariant checks.Any factory-created org can call this for an arbitrary address. Add guards like:
- require Organisation(msg.sender).checkMembership(_member)
- require !Organisation(msg.sender).checkOwnership(_member)
to keep indexes aligned with on-chain truth.
158-214: Detail fetcher is robust; consider lighter payload option.try/catch fallback is good. Optionally add a variant that returns only addresses + counts to cut calldata/gas for UIs that don’t need full owners/members arrays.
243-310: Owner-indexed pagination LGTM; note N external calls.Logic is correct and matches tests (including limit=0 returns empty). Be aware this does N external calls to child orgs; acceptable for view endpoints, but consider a lighter address-only variant for larger pages.
312-379: Member-indexed pagination LGTM; same perf note.Mirrors owner flow correctly and handles limit=0. Same suggestion about an address-only variant.
381-389: Prefer internal helper over external self-call.Use an internal/private function to avoid external call overhead.
391-399: Same: internal helper would be cheaper.Mirror note from getMyOrganisationsAsOwner.
test/OrganisationFactory.t.sol (1)
397-409: Zero limit behavior asserted. Add negative case tests.Nice guard. Please add tests for:
- User leaves org → removed from member index.
- Owner removes member → member index pruned.
These will fail today (no factory hooks) and will guide the fixes suggested in the factory review.src/Organisation.sol (3)
178-211: Pagination logic correct; consider input harmonization.Works as intended. Optional: align with ByStatus variants by rejecting limit == 0 or enforcing paginationLimit for consistency.
333-366: Tree proposals pagination OK; same optional consistency note.Matches verification requests logic; consider consistent limit validation with paginationLimit.
461-462: Factory callback is the right place; ensure lifecycle symmetry.Calling organisationFactoryContract.promoteToOwner(newOwner) keeps indexes up-to-date on promotion. Please add similar callbacks for leaveOrganisation/removeMember to keep member/owner indexes in sync (see factory review).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
script/DeployAllContracts.s.sol(0 hunks)src/Organisation.sol(5 hunks)src/OrganisationFactory.sol(7 hunks)src/TreeNft.sol(3 hunks)test/OrganisationFactory.t.sol(3 hunks)test/TreeNft.t.sol(9 hunks)
💤 Files with no reviewable changes (1)
- script/DeployAllContracts.s.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-26T10:22:04.651Z
Learnt from: ceilican
PR: StabilityNexus/Treee-Solidity#4
File: src/Organisation.sol:4-4
Timestamp: 2025-05-26T10:22:04.651Z
Learning: The Organisation contract in src/Organisation.sol uses a multi-owner design pattern with `address[] public owners` and custom ownership checking, which is incompatible with OpenZeppelin's single-owner Ownable contract. The multi-owner system is required for voting mechanisms on verification requests.
Applied to files:
src/OrganisationFactory.sol
🔇 Additional comments (17)
src/TreeNft.sol (1)
269-279: Good state sync for user verifications during removal.Mirroring the hidden flag into s_userToVerifications keeps public views consistent after removal. This closes a visibility inconsistency.
src/OrganisationFactory.sol (1)
151-153: Good: explicit revert for unknown organisation.Clearer failure mode for bad inputs.
test/OrganisationFactory.t.sol (11)
103-114: Tests updated for paginated getMyOrganisations are good.Covers count and page basics.
126-129: Detail retrieval assertions look solid.Validates totalCount and basic fields.
151-154: IDs pagination smoke test OK.Confirms count and list size.
156-225: Owner-indexed tests are comprehensive.Covers listing and pagination windows.
226-249: Member-indexed happy path covered.Ensures owner vs member separation.
251-281: Member pagination windows covered.Looks good.
282-314: Promotion flow tested end-to-end.Validates counts flip; good coverage.
315-335: getMyOrganisationsAsOwner test OK.Asserts ordering and count.
336-354: getMyOrganisationsAsMember test OK.Happy path verified.
356-383: Mixed-role scenario covered.Confirms separation across orgs.
384-396: Empty results case covered.Both roles zero.
src/Organisation.sol (4)
327-331: Good: explicit revert on bad proposal ID.Bounds check is correct.
413-415: Vote bounds check LGTM.Protects against out-of-range proposal IDs pre-vote.
486-505: Members pagination LGTM.Slice math and early exit are correct.
507-526: Owners pagination LGTM.Mirrors members; correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/OrganisationFactory.sol (2)
102-114: Consider removing defensive ownership check.Currently,
addMemberToOrganisationis only invoked fromOrganisation.addMember, which already ensures the user is not a member (line 78 in Organisation.sol checkscheckMembership). Since owners are also members,checkOwnership(_member)at line 109 will always return false in the current flow, making lines 109-110 unreachable. The else branch (lines 111-113) will always execute.If this check is intended for future extensibility, consider adding a comment explaining the design intent. Otherwise, simplify:
function addMemberToOrganisation(address _member) external { - // This function adds a member to an organization (called by Organisation contract) if (!s_isOrganisation[msg.sender]) revert InvalidOrganisation(); if (msg.sender == address(0)) revert OrganisationDoesNotExist(); s_userToOrganisations[_member].push(msg.sender); - - Organisation org = Organisation(msg.sender); - if (org.checkOwnership(_member)) { - s_userToOrganisationsAsOwner[_member].push(msg.sender); - } else { - s_userToOrganisationsAsMember[_member].push(msg.sender); - } + // New members are added to member-only mapping; promotion happens via promoteToOwner + s_userToOrganisationsAsMember[_member].push(msg.sender); }
289-425: Consider extracting shared logic to reduce duplication.
getUserOrganisationsAsOwner(lines 289-356) andgetUserOrganisationsAsMember(lines 358-425) contain nearly identical pagination and detail-fetching logic (~140 lines total). This duplication increases maintenance burden—future changes must be applied to both functions.Refactor into a shared internal helper:
function _getUserOrganisationDetailsByRole( address[] storage roleOrgs, uint256 offset, uint256 limit ) private view returns (OrganisationDetails[] memory orgs, uint256 totalCount) { totalCount = roleOrgs.length; if (offset >= totalCount || limit == 0) { return (new OrganisationDetails[](0), totalCount); } uint256 end = offset + limit; if (end > totalCount) end = totalCount; uint256 resultLength = end - offset; orgs = new OrganisationDetails[](resultLength); for (uint256 i; i < resultLength;) { address orgAddr = roleOrgs[offset + i]; Organisation org = Organisation(orgAddr); try org.getOrganisationInfo() returns ( address orgAddress, string memory name, string memory description, string memory photoIpfsHash, address[] memory owners, address[] memory members, uint256 timeOfCreation ) { orgs[i] = OrganisationDetails({ contractAddress: orgAddress, name: name, description: description, organisationPhoto: photoIpfsHash, owners: owners, members: members, ownerCount: owners.length, memberCount: members.length, isActive: s_isOrganisation[orgAddress], timeOfCreation: timeOfCreation }); } catch { orgs[i] = OrganisationDetails({ contractAddress: orgAddr, name: "ERROR: Unable to fetch", description: "ERROR: Contract call failed", organisationPhoto: "", owners: new address[](0), members: new address[](0), ownerCount: 0, memberCount: 0, isActive: false, timeOfCreation: 0 }); } unchecked { ++i; } } return (orgs, totalCount); } function getUserOrganisationsAsOwner(address _user, uint256 offset, uint256 limit) external view returns (OrganisationDetails[] memory orgs, uint256 totalCount) { return _getUserOrganisationDetailsByRole(s_userToOrganisationsAsOwner[_user], offset, limit); } function getUserOrganisationsAsMember(address _user, uint256 offset, uint256 limit) external view returns (OrganisationDetails[] memory orgs, uint256 totalCount) { return _getUserOrganisationDetailsByRole(s_userToOrganisationsAsMember[_user], offset, limit); }src/Organisation.sol (1)
182-215: Pagination logic is correct; consider gas costs for large datasets.The two-pass approach (count matches, then collect slice) is correct for filtered pagination. However, for organisations with many verification requests, the full iteration could approach gas limits.
For production with large datasets, consider:
- Indexing by status to avoid full iteration
- Off-chain indexing with events for historical queries
- Setting a maximum verificationCounter that can be scanned
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(1 hunks)src/Organisation.sol(9 hunks)src/OrganisationFactory.sol(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-26T10:22:04.651Z
Learnt from: ceilican
PR: StabilityNexus/Treee-Solidity#4
File: src/Organisation.sol:4-4
Timestamp: 2025-05-26T10:22:04.651Z
Learning: The Organisation contract in src/Organisation.sol uses a multi-owner design pattern with `address[] public owners` and custom ownership checking, which is incompatible with OpenZeppelin's single-owner Ownable contract. The multi-owner system is required for voting mechanisms on verification requests.
Applied to files:
src/OrganisationFactory.sol
🔇 Additional comments (14)
src/OrganisationFactory.sol (7)
14-15: Role-indexed mappings with lifecycle support added.The role-specific mappings introduced here are properly synchronized throughout the lifecycle via
promoteToOwnerandremoveMemberFromOrganisationhooks called from the Organisation contract. This addresses the previous review concern about stale mappings.
116-135: LGTM! Efficient role promotion implementation.The swap-and-pop pattern is gas-efficient, and the unchecked increment is safe for array iteration. The function correctly updates role-specific mappings when a member becomes an owner.
137-180: LGTM! Complete removal logic with role awareness.The function correctly removes members from both the general mapping and the appropriate role-specific mapping. The
_wasOwnerflag is computed by the Organisation contract before removal, ensuring accurate cleanup.
45-94: LGTM! Pagination implemented correctly.The pagination logic properly handles edge cases:
- Returns empty array with totalCount when offset is out of bounds
- Correctly caps the end index at totalCount
- Provides totalCount for client-side pagination UI
196-198: LGTM! Added validation for organisation existence.The revert check prevents attempting to fetch information from unrecognized organisation addresses, improving error handling.
203-259: LGTM! Resilient paginated details fetcher.The try-catch pattern ensures that failures in individual organisation contracts don't break the entire query, returning placeholder error data for failed calls. The pagination logic is consistent with other functions.
427-445: LGTM! Clean delegation pattern for caller convenience.These wrapper functions provide a convenient API for callers to query their own organisations without explicitly passing their address.
src/Organisation.sol (7)
84-112: LGTM! Proper factory integration for member removal.The
wasOwnerflag is correctly computed before modifying the arrays (line 92), ensuring accurate role information is passed to the factory'sremoveMemberFromOrganisationcall (line 110). This maintains consistency between Organisation and OrganisationFactory state.
114-136: LGTM! Consistent factory integration in removeMember.The implementation mirrors
leaveOrganisation—capturing thewasOwnerflag (line 119) before state changes and calling the factory (line 134) to maintain synchronized role mappings.
176-178: Critical bug fix: bounds check now reachable.The previous condition
verificationID >= s_verificationCounter && verificationID < 0was unreachable becauseuint256cannot be negative, making the entire check always false. The fix correctly validates only the upper bound, ensuring invalid IDs now properly revert.This addresses the critical bug flagged in the previous review.
331-333: LGTM! Consistent bounds check fixes for proposal IDs.The same critical fix applied to verification IDs has been consistently applied to proposal ID validation in
getTreePlantingProposal(lines 331-333) andvoteOnTreePlantingProposal(lines 417-419).Also applies to: 417-419
337-370: LGTM! Consistent pagination for tree proposals.The implementation follows the same correct two-pass pattern as
getVerificationRequests. The same gas considerations for large datasets apply here as well.
458-466: LGTM! Proper factory notification on promotion.After adding the member to the owners array (line 464), the factory is correctly notified (line 465) so it can update the role-specific mappings from member-only to owner.
490-530: LGTM! Clean pagination for members and owners.Both
getMembersandgetOwnersimplement straightforward array slicing with proper bounds checking. Since no filtering is required, these are more gas-efficient than the filtered pagination functions.
IronJam11
force-pushed
the
functionPaginations
branch
6 times, most recently
from
a6318f0 to
07f2790
Compare
IronJam11
force-pushed
the
functionPaginations
branch
from
07f2790 to
a99b560
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/TreeNft.sol (3)
168-193: Add limit validation to prevent DoS.This function lacks validation on the
limitparameter, allowing potentially unbounded array allocation and iteration.Apply this diff:
returns (Tree[] memory trees, uint256 totalCount) { // Get the total number of NFTs for this user + if (limit > 50) revert PaginationLimitExceeded();
397-421: Add limit validation to prevent DoS.This function lacks validation on the
limitparameter.Apply this diff:
returns (Tree[] memory trees, uint256 totalCount) { // Get the total number of trees verified by this verifier + if (limit > 50) revert PaginationLimitExceeded();
423-460: Add limit validation to prevent DoS.While the filtering logic for hidden verifications is well-implemented, this function lacks validation on the
limitparameter.Apply this diff:
returns (TreeNftVerification[] memory verifications, uint256 totalCount, uint256 visiblecount) { // This function retrieves all verifiers for a specific tree with pagination + if (limit > 50) revert PaginationLimitExceeded();
🧹 Nitpick comments (5)
src/Organisation.sol (4)
182-215: Add validation for limit parameter.The pagination logic is sound, but there's no validation that
limitis non-zero or reasonable. While view functions don't consume gas when called externally, alimitof 0 would cause unnecessary iteration. Consider adding a check likeif (limit == 0) revert InvalidInput();for consistency withgetVerificationRequestsByStatus(line 222).function getVerificationRequests(uint256 status, uint256 offset, uint256 limit) external view returns (OrganisationVerificationRequest[] memory requests, uint256 totalCount) { + if (limit == 0) revert InvalidInput(); uint256 matchCount = 0;
337-370: Add validation for limit parameter.Same issue as
getVerificationRequests- missing validation thatlimitis non-zero. Consider adding validation for consistency withgetTreePlantingProposalsByStatus(line 379).function getTreePlantingProposals(uint256 status, uint256 offset, uint256 limit) external view returns (TreePlantingProposal[] memory proposals, uint256 totalCount) { + if (limit == 0) revert InvalidInput(); uint256 matchCount = 0;
490-509: Add validation for limit parameter.The pagination logic is correct, but consider adding validation that
limit > 0for consistency and to prevent unnecessary processing.function getMembers(uint256 offset, uint256 limit) external view returns (address[] memory memberList, uint256 totalCount) { + if (limit == 0) revert InvalidInput(); totalCount = members.length;
511-530: Add validation for limit parameter.Same issue as
getMembers- consider adding validation thatlimit > 0.function getOwners(uint256 offset, uint256 limit) external view returns (address[] memory ownerList, uint256 totalCount) { + if (limit == 0) revert InvalidInput(); totalCount = owners.length;src/OrganisationFactory.sol (1)
271-274: Simplify boolean comparison.The explicit
== falsecomparison is unnecessary.- if (s_isOrganisation[_organisationAddress] == false) { + if (!s_isOrganisation[_organisationAddress]) { revert OrganisationDoesNotExist(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Organisation.sol(9 hunks)src/OrganisationFactory.sol(7 hunks)src/TreeNft.sol(6 hunks)test/TreeNft.t.sol(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/TreeNft.t.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-26T10:22:04.651Z
Learnt from: ceilican
PR: StabilityNexus/Treee-Solidity#4
File: src/Organisation.sol:4-4
Timestamp: 2025-05-26T10:22:04.651Z
Learning: The Organisation contract in src/Organisation.sol uses a multi-owner design pattern with `address[] public owners` and custom ownership checking, which is incompatible with OpenZeppelin's single-owner Ownable contract. The multi-owner system is required for voting mechanisms on verification requests.
Applied to files:
src/Organisation.solsrc/OrganisationFactory.sol
🔇 Additional comments (21)
src/Organisation.sol (6)
92-92: LGTM! Correct role tracking for factory notification.The
wasOwnerflag is properly captured before member removal and passed to the factory, enabling the factory to update its role-specific mappings correctly.Also applies to: 110-110
119-119: LGTM! Consistent role tracking in removeMember.The same
wasOwnerpattern is correctly applied here, maintaining consistency with theleaveOrganisationflow.Also applies to: 134-134
176-178: LGTM! Bounds check correctly simplified.The previous buggy condition (
&& verificationID < 0on an unsigned int) has been properly removed. The check now correctly validates only the upper bound.
331-333: LGTM! Bounds check correctly simplified.The same fix as applied to verification requests - correctly validates only the upper bound.
417-419: LGTM! Consistent bounds check.Correctly validates the upper bound before accessing the mapping.
465-465: LGTM! Correct factory integration for role promotion.The call to
organisationFactoryContract.promoteToOwner(newOwner)after adding to the localownersarray ensures the factory's role-specific mappings stay synchronized.src/OrganisationFactory.sol (14)
14-15: LGTM! Good addition of role-specific mappings.These private mappings enable efficient querying of organisations by user role. The separation between owner and member-only tracking is a clean design.
38-38: LGTM! Correct initialization of owner mapping.The creator is properly tracked in the owner-specific mapping at organisation creation. Note that they're not added to
s_userToOrganisationsAsMembersince that mapping is for non-owner members only.
102-114: LGTM! Defensive role checking in member addition.The function correctly checks whether the added member is an owner and tracks them in the appropriate role-specific mapping. The check on line 109 is defensive—in normal flow,
addMemberis only called for new non-owner members, so this branch might rarely execute.
116-135: LGTM! Clean role promotion logic with gas optimization.The function correctly removes the member from the member-only array and adds them to the owner array. The use of
uncheckedincrement is appropriate for gas savings in a bounded loop. If the member is not found in the array (edge case), the function silently continues, which is acceptable since they'll be added to the owner array regardless.
137-180: LGTM! Comprehensive member removal with role handling.The function properly handles removal from both the general mapping and the appropriate role-specific mapping based on the
_wasOwnerflag. The use ofuncheckedincrements is appropriate for gas optimization.
196-198: LGTM! Good defensive validation.Adding the existence check prevents returning stale data for unregistered organisations.
45-64: LGTM! Clean pagination implementation.The pagination logic correctly handles edge cases (offset >= totalCount) and returns both the slice and total count. No validation on
limitis needed here since view functions don't consume gas when called externally.
67-73: LGTM! Clean delegation pattern.Correctly delegates to the paginated
getUserOrganisationswithmsg.sender.
75-94: LGTM! Consistent pagination implementation.Follows the same clean pattern as
getUserOrganisations.
203-259: LGTM! Robust error handling in batch operation.The use of try-catch to handle individual organisation call failures is good defensive programming. Returning placeholder error data prevents one failing organisation from breaking the entire batch query.
289-356: LGTM! Well-implemented role-specific paginated query.The function correctly retrieves organisations where the user is an owner, with proper pagination, error handling via try-catch, and gas optimization via unchecked increments. The pattern is clean and consistent.
358-425: LGTM! Consistent with owner query implementation.Follows the same robust pattern as
getUserOrganisationsAsOwnerbut for member-only organisations.
427-435: LGTM! Clean wrapper for caller's owner organisations.Correctly delegates to the user-specific function with
msg.sender.
437-445: LGTM! Clean wrapper for caller's member organisations.Correctly delegates to the user-specific function with
msg.sender.src/TreeNft.sol (1)
224-226: Approve the verification state tracking.The verification tracking correctly updates all necessary mappings including the newly added
s_userToVerificationsmapping.
…dity into functionPaginations merge main with functionspaginations
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/TreeNft.sol (3)
242-324: Critical: Reset verification mapping to allow re-verification.
s_tokenIDtoUserVerification[_tokenId][verifier]is never reset tofalseafter removal. This permanently prevents the verifier from verifying this tree again, even if the removal was a mistake or new proof becomes available.Add this line after marking the verification as hidden (around line 287):
if (treeNftVerification.verifier == verifier && !treeNftVerification.isHidden) { treeNftVerification.isHidden = true; + s_tokenIDtoUserVerification[_tokenId][verifier] = false; // Also update the verification in the user's array
326-398: Critical: Reset verification mapping to allow re-verification.Like
removeVerification, this function never resetss_tokenIDtoUserVerification[tokenId][verifier]tofalse. The verifier cannot re-verify the tree after removal.Add this line after marking verifications as hidden (around line 371):
verification.isHidden = true; userVerifications[_userVerificationIndex].isHidden = true; + s_tokenIDtoUserVerification[tokenId][verifier] = false; User storage user = s_addressToUser[verifier];
513-545: Add limit validation and zero-address check.This function has two issues:
- Missing limit validation — No check against
maxLimitForPagination, enabling DoS via unbounded queries.- No zero-address check — Line 533 creates a
PlanterTokeninstance without verifying the address is non-zero. Whileverify()always initializes a PlanterToken, defensive programming suggests validating.Apply this diff:
function getUserVerifierTokenDetails(address userAddress, uint256 offset, uint256 limit) public view returns (VerificationDetails[] memory verifierTokenDetails, uint256 totalCount) { // This function returns the verifier token details of the user with pagination + if (limit > maxLimitForPagination) revert MaximumLimitRequestExceeded(); TreeNftVerification[] memory userVerifications = s_userToVerifications[userAddress]; totalCount = userVerifications.length; if (offset >= totalCount) { return (new VerificationDetails[](0), totalCount); } uint256 end = offset + limit; if (end > totalCount) { end = totalCount; } uint256 resultLength = end - offset; verifierTokenDetails = new VerificationDetails[](resultLength); for (uint256 i = 0; i < resultLength; i++) { uint256 verificationIndex = offset + i; - PlanterToken planterToken = PlanterToken(userVerifications[verificationIndex].verifierPlanterTokenAddress); + address planterTokenAddr = userVerifications[verificationIndex].verifierPlanterTokenAddress; + uint256 numberOfTrees = 0; + if (planterTokenAddr != address(0)) { + PlanterToken planterToken = PlanterToken(planterTokenAddr); + numberOfTrees = planterToken.balanceOf(userAddress); + } verifierTokenDetails[i] = VerificationDetails({ verifier: userVerifications[verificationIndex].verifier, timestamp: userVerifications[verificationIndex].timestamp, proofHashes: userVerifications[verificationIndex].proofHashes, description: userVerifications[verificationIndex].description, isHidden: userVerifications[verificationIndex].isHidden, - numberOfTrees: planterToken.balanceOf(userAddress), + numberOfTrees: numberOfTrees, verifierPlanterTokenAddress: userVerifications[verificationIndex].verifierPlanterTokenAddress }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)src/Organisation.sol(11 hunks)src/OrganisationFactory.sol(6 hunks)src/TreeNft.sol(8 hunks)src/utils/errors.sol(1 hunks)test/TreeNft.t.sol(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-26T10:22:04.651Z
Learnt from: ceilican
PR: StabilityNexus/Treee-Solidity#4
File: src/Organisation.sol:4-4
Timestamp: 2025-05-26T10:22:04.651Z
Learning: The Organisation contract in src/Organisation.sol uses a multi-owner design pattern with `address[] public owners` and custom ownership checking, which is incompatible with OpenZeppelin's single-owner Ownable contract. The multi-owner system is required for voting mechanisms on verification requests.
Applied to files:
src/OrganisationFactory.sol
🔇 Additional comments (7)
src/utils/errors.sol (1)
48-48: LGTM!The new
MaximumLimitRequestExceedederror provides clearer semantics for pagination limit violations.README.md (1)
1-177: LGTM!The README has been significantly improved with better organization, social badges, and clearer quickstart instructions.
test/TreeNft.t.sol (1)
488-636: LGTM!Comprehensive test coverage for
getUserVerifierTokenDetailsPaginated, including edge cases (empty results, boundaries, hidden verifications).src/OrganisationFactory.sol (4)
14-15: LGTM!The role-specific mappings enable efficient filtering of organisations by user role, addressing the past review comment about maintaining role-indexed data.
120-139: LGTM!
promoteToOwnercorrectly removes the member from the member-only array and adds them to the owner array, ensuring role mappings stay synchronized with Organisation state.
141-184: LGTM!
removeMemberFromOrganisationproperly cleans up both general and role-specific mappings using swap-and-pop. The_wasOwnerparameter ensures correct array cleanup.
287-355: LGTM!
getUserOrganisationsAsOwnerprovides paginated access to organisations where the user has owner privileges, with proper error handling via try-catch for failed contract calls.
| function getMembers(uint256 offset, uint256 limit) | ||
| external | ||
| view | ||
| returns (address[] memory memberList, uint256 totalCount) | ||
| { | ||
| totalCount = members.length; | ||
| if (offset >= totalCount) { | ||
| return (new address[](0), totalCount); | ||
| } | ||
| uint256 end = offset + limit; | ||
| if (end > totalCount) { | ||
| end = totalCount; | ||
| } | ||
| uint256 resultLength = end - offset; | ||
| memberList = new address[](resultLength); | ||
| for (uint256 i = 0; i < resultLength; i++) { | ||
| memberList[i] = members[offset + i]; | ||
| } | ||
| return (memberList, totalCount); | ||
| } |
There was a problem hiding this comment.
Add limit validation to prevent unbounded queries.
getMembers lacks a paginationLimit check, unlike getOwners (line 520). An attacker could pass an extremely large limit, causing excessive gas consumption.
Apply this diff to add validation:
function getMembers(uint256 offset, uint256 limit)
external
view
returns (address[] memory memberList, uint256 totalCount)
{
+ if (limit > paginationLimit) revert MaximumLimitRequestExceeded();
totalCount = members.length;📝 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.
| function getMembers(uint256 offset, uint256 limit) | |
| external | |
| view | |
| returns (address[] memory memberList, uint256 totalCount) | |
| { | |
| totalCount = members.length; | |
| if (offset >= totalCount) { | |
| return (new address[](0), totalCount); | |
| } | |
| uint256 end = offset + limit; | |
| if (end > totalCount) { | |
| end = totalCount; | |
| } | |
| uint256 resultLength = end - offset; | |
| memberList = new address[](resultLength); | |
| for (uint256 i = 0; i < resultLength; i++) { | |
| memberList[i] = members[offset + i]; | |
| } | |
| return (memberList, totalCount); | |
| } | |
| function getMembers(uint256 offset, uint256 limit) | |
| external | |
| view | |
| returns (address[] memory memberList, uint256 totalCount) | |
| { | |
| if (limit > paginationLimit) revert MaximumLimitRequestExceeded(); | |
| totalCount = members.length; | |
| if (offset >= totalCount) { | |
| return (new address[](0), totalCount); | |
| } | |
| uint256 end = offset + limit; | |
| if (end > totalCount) { | |
| end = totalCount; | |
| } | |
| uint256 resultLength = end - offset; | |
| memberList = new address[](resultLength); | |
| for (uint256 i = 0; i < resultLength; i++) { | |
| memberList[i] = members[offset + i]; | |
| } | |
| return (memberList, totalCount); | |
| } |
🤖 Prompt for AI Agents
In src/Organisation.sol around lines 492 to 511, getMembers currently accepts an
unchecked limit which allows callers to request an arbitrarily large page and
may cause excessive gas; add the same pagination limit validation used in
getOwners (around line 520) by requiring the supplied limit is > 0 and <=
paginationLimit (or if zero should be treated as a max, enforce accordingly),
reverting with a clear message like "limit too large" when exceeded; keep the
rest of the pagination logic intact so offset/limit trimming still occurs after
the validation.
| treeNft.removeVerification(0, verifier1); | ||
| assertFalse(treeNft.isVerified(0, verifier1)); | ||
| TreeNftVerification[] memory verifications = treeNft.getTreeNftVerifiers(0); | ||
| assertTrue(treeNft.isVerified(0, verifier1)); |
There was a problem hiding this comment.
Test assertion confirms the re-verification bug.
This assertion expects isVerified to return true after removeVerification, which reveals the underlying bug where s_tokenIDtoUserVerification is never reset. Once the contract bug is fixed (resetting the mapping to false on removal), this test will need updating.
After fixing the contract, update this assertion:
- assertTrue(treeNft.isVerified(0, verifier1));
+ assertFalse(treeNft.isVerified(0, verifier1));Same issue applies to lines 166, 271, 445, 481, and 652.
📝 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.
| assertTrue(treeNft.isVerified(0, verifier1)); | |
| assertFalse(treeNft.isVerified(0, verifier1)); |
🤖 Prompt for AI Agents
In test/TreeNft.t.sol at line 134 (and similarly update assertions at lines 166,
271, 445, 481, and 652), the test currently asserts that isVerified(tokenId,
verifier) returns true after removeVerification, which matches a known contract
bug; after fixing the contract to reset
s_tokenIDtoUserVerification[tokenId][verifier] = false on removal, change these
tests to assertFalse(isVerified(...)) (or assertEq to false) to reflect the
corrected behavior so the tests validate removal correctly.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests
Chores
Documentation