Minor bug fixes #10
Minor bug fixes #10
Conversation
WalkthroughAdds combined and single-target deployment scripts; removes VerifierToken; introduces per-user PlanterToken creation, per-token verifications, and expanded tree metadata/numberOfTrees support; updates Organisation, structs/errors, tests, PlanterToken, deployment scripts, and CI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant Script as DeployAllContractsAtOnce
participant Care as CareToken
participant Leg as LegacyToken
participant Tree as TreeNft
participant OrgF as OrganisationFactory
Dev->>Script: run()
Script->>Care: deploy CareToken(deployer)
Script->>Leg: deploy LegacyToken(deployer)
Script->>Tree: deploy TreeNft(address(Care), address(Leg))
Script->>Care: transferOwnership(Tree)
Script->>Leg: transferOwnership(Tree)
Script->>OrgF: deploy OrganisationFactory(address(Tree))
Script->>Tree: verifyDeployment()
Script-->>Dev: log addresses & summary
sequenceDiagram
autonumber
actor Minter
actor Verifier
participant Tree as TreeNft
participant PT as PlanterToken (per-verifier)
Minter->>Tree: mintNft(..., qrPhoto, metadata, ..., numberOfTrees)
Tree-->>Minter: tokenId
Verifier->>Tree: verify(tokenId, proofHashes, description)
Tree->>Tree: revert if caller is owner (CannotVerifyOwnTree)
alt has PlanterToken for verifier
Tree-->>PT: use existing planter token
else
Tree->>PT: deploy new PlanterToken(verifier)
end
Tree->>PT: mint planter tokens (based on numberOfTrees)
Tree-->>Verifier: record verification
Minter->>Tree: removeVerification(tokenId, verifier)
Tree->>Tree: find verification or revert (VerificationNotFound)
Tree->>PT: burn tokens if applicable
Tree-->>Minter: emit VerificationRemoved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
src/Organisation.sol (4)
172-174: Critical: unreachable revert makes getVerificationRequest accept invalid IDs.
ConditionverificationID >= s_verificationCounter && verificationID < 0is never true (uint can’t be < 0). Use a single upper-bound check.Apply:
- if (verificationID >= s_verificationCounter && verificationID < 0) revert InvalidVerificationId(); + if (verificationID >= s_verificationCounter) revert InvalidVerificationId();
265-266: Latitude/longitude bounds are incorrect.
Latitude should be ≤ 90° and longitude ≤ 180° (scaled). Current checks allow up to 180°/360°.Suggested fix:
- if (_latitude > 180 * 1e6) revert InvalidCoordinates(); - if (_longitude > 360 * 1e6) revert InvalidCoordinates(); + if (_latitude > 90 * 1e6) revert InvalidCoordinates(); + if (_longitude > 180 * 1e6) revert InvalidCoordinates();Consider mirroring the same fix in TreeNft.mintNft for consistency.
246-252: Rejection threshold likely too low.
nothreshold uses(owners.length - requiredVotes), which for 3 owners equals 1 — a single No can reject. Usually both approval and rejection should userequiredVotes.Apply:
- } else if (s_verificationNoVoters[verificationID].length >= (owners.length - requiredVotes)) { + } else if (s_verificationNoVoters[verificationID].length >= requiredVotes) { request.status = 2; }
398-407: Fix test mintNft argument order (geoHash / metadata swapped)
- src/Organisation.sol: approve path forwards proposal.metadata to mintNft — calls at ~285–293 and ~398–407.
- src/TreeNft.sol: mintNft signature expects metadata before geoHash (function at line ~69).
- test/TreeNft.t.sol: line 405 calls mintNft(..., GEO_HASH, METADATA, ...) — change to (..., METADATA, GEO_HASH, ...) to match signature.
🧹 Nitpick comments (9)
src/utils/errors.sol (1)
4-4: Typo in docstring.“Organisaiton” → “Organisation”.
Apply:
-/// Organisaiton Factory and Organisation +/// Organisation Factory and Organisationsrc/utils/structs.sol (1)
46-57: UserDetails duplicates User fields; consider composition to avoid drift.Keep
Useras the stored struct and deriveUserDetailson read paths, or embedUserinsideUserDetailsto prevent divergence. If stored on-chain, consider narrower types for token counters to pack better.script/DeployOrganisationFactory.s.sol (1)
31-36: Optional post-deploy verification.Consider asserting the factory is wired to the provided TreeNFT to catch misconfiguration early.
Example:
console.log("\n========== DEPLOYMENT SUMMARY =========="); console.log("OrganisationFactory Address: ", orgFactoryAddress); console.log("Linked TreeNFT Address: ", treeNftAddress); console.log("Deployment completed successfully."); console.log("========================================\n"); + + // Quick sanity check (non-reverting, logs only) + // OrganisationFactory org = OrganisationFactory(orgFactoryAddress); + // console.log("Factory points to TreeNFT: ", address(org.treeNftContract()));test/Organisation.t.sol (1)
244-245: plantTreeProposal arg order looks correct; add a metadata assertion.
Consider asserting proposal.metadata to catch regressions.Apply this diff to extend the test:
TreePlantingProposal memory proposal = Organisation(orgAddress).getTreePlantingProposal(0); vm.stopPrank(); assertEq(proposal.id, 0); assertEq(proposal.status, 1); assertEq(proposal.latitude, LATITUDE); assertEq(proposal.longitude, LONGITUDE); assertEq(proposal.species, SPECIES); assertEq(proposal.imageUri, IMAGE_URI); assertEq(proposal.qrIpfsHash, QR_IPFS_HASH); + assertEq(proposal.metadata, METADATA); assertEq(proposal.geoHash, GEOHASH);script/DeployAllContracts.s.sol (2)
52-61: Ownership transfers logged; verify all four owners in verifyDeployment.
You only assert CareToken’s owner. Add checks for the other tokens.Apply:
CareToken careToken = CareToken(careTokenAddress); require(careToken.owner() == treeNftAddress, "CareToken ownership not transferred"); + PlanterToken planterToken = PlanterToken(planterTokenAddress); + require(planterToken.owner() == treeNftAddress, "PlanterToken ownership not transferred"); + VerifierToken verifierToken = VerifierToken(verifierTokenAddress); + require(verifierToken.owner() == treeNftAddress, "VerifierToken ownership not transferred"); + LegacyToken legacy = LegacyToken(legacyTokenAddress); + require(legacy.owner() == treeNftAddress, "LegacyToken ownership not transferred");
62-66: Persist OrganisationFactory address if needed.
Optional: expose orgFactoryAddress as a public state var for tooling.- function run() external { + address public orgFactoryAddress; + function run() external { ... - address orgFactoryAddress = address(orgFactory); + orgFactoryAddress = address(orgFactory);src/TreeNft.sol (3)
406-408: Redundant check in _exists.
tokenId >= 0is always true for uint; simplify.- function _exists(uint256 tokenId) internal view returns (bool) { - return tokenId < s_tokenCounter && tokenId >= 0; - } + function _exists(uint256 tokenId) internal view returns (bool) { + return tokenId < s_tokenCounter; + }
310-347: Naming nit: visiblecount vs visibleCount.
Return variablevisiblecountis lowercased; local usesvisibleCount. Align names for clarity.- returns (TreeNftVerification[] memory verifications, uint256 totalCount, uint256 visiblecount) + returns (TreeNftVerification[] memory verifications, uint256 totalCount, uint256 visibleCount)
109-134: Optional: include metadata in tokenURI.
Improves discoverability on-chain wallets/UIs.Example addition inside attributes array:
- Add {"trait_type":"Metadata","value": tree.metadata} or embed as an external_url field if it’s an IPFS URI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (9)
script/DeployAllContracts.s.sol(1 hunks)script/DeployOrganisationFactory.s.sol(1 hunks)script/DeployTreeNftContract.s.sol(1 hunks)src/Organisation.sol(4 hunks)src/TreeNft.sol(5 hunks)src/utils/errors.sol(2 hunks)src/utils/structs.sol(2 hunks)test/Organisation.t.sol(6 hunks)test/TreeNft.t.sol(22 hunks)
🔇 Additional comments (21)
src/utils/errors.sol (2)
30-30: Good addition: deploy-time contract address guard.
InvalidContractAddress()is a sensible, reusable error for script/runtime validation.
51-54: Import custom errors in deployment scriptsDeployment scripts reference custom errors from src/utils/errors.sol — ensure those scripts import them to avoid compile failures.
- script/DeployTreeNftContract.s.sol — lines 79, 82, 85, 88:
revert OwnershipNotTransferred();- script/DeployOrganisationFactory.s.sol — line 14:
revert InvalidContractAddress();Add an import, for example:
import { OwnershipNotTransferred, InvalidContractAddress } from "../src/utils/errors.sol";src/utils/structs.sol (3)
68-68: Metadata field on Tree: LGTM.Adds flexibility for per‑token metadata storage and matches test updates.
85-85: TreePlantingProposal.metadata: LGTM.Consistent with Tree metadata propagation.
60-60: Tree.id semantics — confirmed aligned with tokenId. s_tokenIDtoTree[tokenId] is initialized with Tree(tokenId, ...) during mint (no other .id writes/usages found); getTreeDetailsbyID returns s_tokenIDtoTree[tokenId]. (src/utils/structs.sol:59-61; src/TreeNft.sol:88-90,206-211)test/TreeNft.t.sol (2)
311-316: Event expectation + visibility filtering behavior: LGTM.Emitting
VerificationRemovedand asserting the filtered list length is 0 matches the new isHidden approach.
69-70: Fix swapped args in mintNft call (test/TreeNft.t.sol:405)mintNft expects (…, qrIpfsHash, metadata, geoHash, …); the call at line 405 passes GEO_HASH then METADATA, swapping those fields and corrupting stored data.
- treeNft.mintNft(LATITUDE, LONGITUDE, SPECIES, IMAGE_URI, QR_IPFS_HASH, GEO_HASH, METADATA, initialPhotos); + treeNft.mintNft(LATITUDE, LONGITUDE, SPECIES, IMAGE_URI, QR_IPFS_HASH, METADATA, GEO_HASH, initialPhotos);Add assertEq(tree.metadata, METADATA) in a couple of tests and recheck the gas threshold in testGasOptimization after this fix.
Likely an incorrect or invalid review comment.
test/Organisation.t.sol (5)
44-44: Good: central METADATA test constant.
Keeps tests DRY and aligned with new signatures.
162-163: mintNft arg order updated correctly.
Order matches TreeNft.mintNft(..., qrIpfsHash, metadata, geoHash, photos).
194-196: mintNft updated here as well.
Consistent with the new metadata slot.
274-276: mintNft arg order updated consistently.
No issues.
297-299: plantTreeProposal update consistent with metadata addition.
Looks good.src/Organisation.sol (3)
261-264: API: metadata parameter correctly introduced.
Signature change is coherent with tests and TreeNft.
279-280: Proposal now stores metadata.
Matches structs and tests.
285-295: Auto-approve: mint path forwards metadata — organisation calls updated; tests need one fix.
src/Organisation.sol call sites (lines 285, 398) now pass metadata in the correct position matching src/TreeNft.sol:mintNft. One test still passes geoHash and metadata swapped: test/TreeNft.t.sol:405 — swap those args and re-run tests.script/DeployAllContracts.s.sol (1)
12-18: State captures for deployed addresses are helpful.
Good for post-run inspection.src/TreeNft.sol (5)
75-78: API: metadata added to mintNft.
Matches tests and Organisation flow.
89-104: Tree now stores id and metadata.
Initialization order is correct; id == tokenId maintained.
374-390: User profile aggregation looks correct.
Balances and counters are assembled as expected; revert on unregistered is fine.
410-412: ping utility is fine.
Simple liveness check.
1-414: No duplicate getUserProfile declaration found — single definition
Single declaration at src/TreeNft.sol:374 (function getUserProfile). No other declarations found repo-wide.
| if (treeNftAddress.code.length <= 0) revert InvalidContractAddress(); | ||
|
|
There was a problem hiding this comment.
Missing import for InvalidContractAddress(); script won’t compile.
Add the errors import.
Apply:
import "../src/OrganisationFactory.sol";
+import "../src/utils/errors.sol";📝 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.
| if (treeNftAddress.code.length <= 0) revert InvalidContractAddress(); | |
| import "../src/OrganisationFactory.sol"; | |
| import "../src/utils/errors.sol"; |
🤖 Prompt for AI Agents
In script/DeployOrganisationFactory.s.sol around lines 14-15, the revert
InvalidContractAddress() is used but the error type is not imported so the
script will not compile; add an import for the InvalidContractAddress error from
your project's errors file (e.g., errors.sol) at the top of the solidity script,
ensuring the import path is correct for the repository structure, then recompile
to confirm the symbol is resolved.
| if (careToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | ||
|
|
||
| PlanterToken planterToken = PlanterToken(planterTokenAddress); | ||
| if (planterToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | ||
|
|
||
| VerifierToken verifierToken = VerifierToken(verifierTokenAddress); | ||
| if (verifierToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | ||
|
|
||
| LegacyToken legacyToken = LegacyToken(legacyTokenAddress); | ||
| if (legacyToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | ||
|
|
There was a problem hiding this comment.
Missing import for custom error causes compilation failure.
OwnershipNotTransferred() is undeclared in this source unit.
Apply:
import "../src/token-contracts/LegacyToken.sol";
+import "../src/utils/errors.sol";Optional: stop broadcasting before view-only verification to avoid accidental txs during checks.
- console.log("========================\n");
- verifyDeployment();
+ console.log("========================\n");
+ vm.stopBroadcast();
+ verifyDeployment();📝 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.
| if (careToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | |
| PlanterToken planterToken = PlanterToken(planterTokenAddress); | |
| if (planterToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | |
| VerifierToken verifierToken = VerifierToken(verifierTokenAddress); | |
| if (verifierToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | |
| LegacyToken legacyToken = LegacyToken(legacyTokenAddress); | |
| if (legacyToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | |
| // --- add to imports (near other imports) --- | |
| import "../src/token-contracts/LegacyToken.sol"; | |
| import "../src/utils/errors.sol"; | |
| // --- existing ownership checks (unchanged) --- | |
| if (careToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | |
| PlanterToken planterToken = PlanterToken(planterTokenAddress); | |
| if (planterToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | |
| VerifierToken verifierToken = VerifierToken(verifierTokenAddress); | |
| if (verifierToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | |
| LegacyToken legacyToken = LegacyToken(legacyTokenAddress); | |
| if (legacyToken.owner() != treeNftAddress) revert OwnershipNotTransferred(); | |
| // --- replace before verification --- | |
| console.log("========================\n"); | |
| vm.stopBroadcast(); | |
| verifyDeployment(); |
🤖 Prompt for AI Agents
In script/DeployTreeNftContract.s.sol around lines 79-89, the custom error
OwnershipNotTransferred() is not declared or imported causing compilation
failures; fix by either importing the contract/file that declares this error at
the top of the Solidity file or add a local declaration `error
OwnershipNotTransferred();` in this source unit, then keep the ownership checks
as-is; optionally, to avoid accidental transactions during view-only
verification, call vm.stopBroadcast() (or the appropriate stopBroadcast function
from the foundry runtime) before performing those read-only owner checks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/TreeNft.sol (2)
233-235: Mint full ERC20 units when rewarding planters.
planterToken.mint(..., tree.numberOfTrees)still mints raw integers, i.e., base units (1 = 1e‑18 tokens with 18 decimals). Planters effectively receive zero whole tokens. Multiply by10 ** uint256(planterToken.decimals())(or1e18) before minting so rewards match the intended tree count.
239-255: Make removal actually revoke verification state.Hiding the record leaves
s_tokenIDtoUserVerification[_tokenId][verifier]true ands_verifierToTokenIDsuntouched, soisVerifiedgates never unlock and revoked verifications still surface in listings. Clear the boolean flag (and prune token ID lists or filter them on read) when marking hidden so removal has effect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/TreeNft.sol(5 hunks)src/token-contracts/PlanterToken.sol(1 hunks)src/utils/errors.sol(2 hunks)src/utils/structs.sol(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
src/utils/structs.sol
[error] 21-25: Code formatting differences detected by forge fmt --check in struct definitions (verifierPlanterTokenAddress field). Ensure alignment and spacing as per project style.
src/TreeNft.sol
[error] 46-50: Constructor formatting and inheritance formatting differences detected by forge fmt --check in TreeNft.sol. Apply consistent formatting.
src/token-contracts/PlanterToken.sol
[error] 6-12: Code formatting differences detected by forge fmt --check in PlanterToken.sol. Ensure consistent spacing and indentation.
src/TreeNft.sol
Outdated
| function getUserVerifierTokenDetails(address userAddress) public view returns (VerificationDetails[] memory verifierTokenDetails) { | ||
| // This function returns the verifier token address of the user | ||
|
|
||
| TreeNftVerification[] memory userVerifications = s_userToVerifications[userAddress]; | ||
| for(uint i = 0; i < userVerifications.length; i++) { | ||
| PlanterToken planterToken = PlanterToken(userVerifications[i].verifierPlanterTokenAddress); | ||
| verifierTokenDetails[i] = VerificationDetails({ | ||
| verifier: userVerifications[i].verifier, | ||
| timestamp: userVerifications[i].timestamp, | ||
| proofHashes: userVerifications[i].proofHashes, | ||
| description: userVerifications[i].description, | ||
| isHidden: userVerifications[i].isHidden, | ||
| numberOfTrees: planterToken.balanceOf(userAddress), | ||
| verifierPlanterTokenAddress: userVerifications[i].verifierPlanterTokenAddress | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix array initialisation and token accounting.
verifierTokenDetails[i] = … writes into an uninitialised memory array, causing every call to revert. Instantiate it first:
verifierTokenDetails = new VerificationDetails[](userVerifications.length);Additionally, you query planterToken.balanceOf(userAddress) even though verify() mints to the tree owner (ownerOf(_tokenId)), so the returned balances are always zero for the verifier. Please fetch balances for the actual recipient (or adjust minting) to report meaningful numbers.
🤖 Prompt for AI Agents
In src/TreeNft.sol around lines 398 to 414, the function writes into an
uninitialised memory array and reports wrong token balances; initialize
verifierTokenDetails with new VerificationDetails[](userVerifications.length)
before the loop, and for numberOfTrees query the planter token balance of the
actual mint recipient (not the verifier): fetch the recipient (e.g.,
ownerOf(userVerifications[i].tokenId) if tokenId is stored on the verification,
or use whatever recipient address the verify() call mints to) and call
planterToken.balanceOf(recipient); alternatively modify the verification data to
store the recipient at verification time and use that stored address for the
balance lookup.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/DeployTreeNftContract.s.sol (1)
4-64: Fix missing import forOwnershipNotTransferred.
OwnershipNotTransferred()is referenced but never declared in this compilation unit, so the script fails to compile. Import the custom error definition fromsrc/utils/errors.sol(or declare it locally) before using it.import "../src/token-contracts/LegacyToken.sol"; +import "../src/utils/errors.sol";
♻️ Duplicate comments (3)
src/TreeNft.sol (3)
207-239: Verify function implements comprehensive validation and token minting.The updated
verify()function includes:
- Self-verification prevention (line 213)
- Dynamic PlanterToken creation for new verifiers (lines 218-221)
- Token minting to tree owner based on numberOfTrees (line 235)
- Proper state updates across multiple mappings
However, there's a critical issue with the token amount being minted.
Token minting uses raw numberOfTrees without accounting for ERC20 decimals.
Line 235 mints
tree.numberOfTreesdirectly, which represents the number of base units (1e-18 tokens per unit if using 18 decimals). If the intent is to mint whole tokens, this should be multiplied by10**planterToken.decimals().Apply this diff if the intent is to mint whole tokens:
- planterToken.mint(ownerOf(_tokenId), tree.numberOfTrees); + planterToken.mint(ownerOf(_tokenId), tree.numberOfTrees * 10**planterToken.decimals());Based on learnings from past review comments.
445-465: Critical: Uninitialized array causes revert.The function writes to
verifierTokenDetails[i]without first initializing the array, which will cause every call to revert.Initialize the array before the loop:
function getUserVerifierTokenDetails(address userAddress) public view returns (VerificationDetails[] memory verifierTokenDetails) { // This function returns the verifier token address of the user TreeNftVerification[] memory userVerifications = s_userToVerifications[userAddress]; + verifierTokenDetails = new VerificationDetails[](userVerifications.length); for (uint256 i = 0; i < userVerifications.length; i++) {Additionally, Line 461 queries
planterToken.balanceOf(userAddress)but the tokens were minted to the tree owner (line 235 inverify()), not the verifier. This will return zero for most queries.To fix the balance lookup, fetch the tree owner's balance:
- numberOfTrees: planterToken.balanceOf(userAddress), + numberOfTrees: planterToken.balanceOf(ownerOf(userVerifications[i].tokenId)),Based on learnings from past review comments.
487-489: Remove test artifact function.The
ping()function appears to be a test artifact and should be removed from production code as it serves no functional purpose.Based on learnings from past review comment by Zahnentferner.
Remove the function:
- function ping() public pure returns (string memory) { - return "pong"; - }
🧹 Nitpick comments (2)
script/DeployAllContracts.s.sol (1)
75-79: Use the correct contract type forlegacyToken.
legacyTokenis instantiated asCareToken, which is misleading and fragile; if the interfaces ever diverge this breaks verification. Instantiate it asLegacyTokeninstead.- CareToken legacyToken = CareToken(legacyTokenAddress); + LegacyToken legacyToken = LegacyToken(legacyTokenAddress);src/TreeNft.sol (1)
483-485: Redundant check in _exists function.Line 484 includes
tokenId >= 0which is always true foruint256types since they cannot be negative.Simplify to:
function _exists(uint256 tokenId) internal view returns (bool) { - return tokenId < s_treeTokenCounter && tokenId >= 0; + return tokenId < s_treeTokenCounter; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
script/DeployAllContracts.s.sol(1 hunks)script/DeployTreeNftContract.s.sol(2 hunks)src/Organisation.sol(4 hunks)src/TreeNft.sol(9 hunks)src/token-contracts/PlanterToken.sol(1 hunks)src/token-contracts/VerifierToken.sol(0 hunks)src/utils/errors.sol(2 hunks)src/utils/structs.sol(3 hunks)test/Organisation.t.sol(6 hunks)test/OrganisationFactory.t.sol(1 hunks)test/TreeNft.t.sol(1 hunks)
💤 Files with no reviewable changes (1)
- src/token-contracts/VerifierToken.sol
🧰 Additional context used
🪛 GitHub Actions: CI
test/Organisation.t.sol
[error] 237-243: forge fmt --check reported formatting changes. The diff shows formatting adjustments to the plantTreeProposal call. Run 'forge fmt' to auto-format or fix manually.
[error] 295-299: forge fmt --check reported formatting changes. The diff shows formatting adjustments to the plantTreeProposal call. Run 'forge fmt' to auto-format or fix manually.
🔇 Additional comments (10)
test/TreeNft.t.sol (6)
4-4: Import path updated for Foundry standard.The import path has been updated to
../lib/forge-std/src/Test.sol, which is the correct path for Foundry's standard library when installed as a git submodule in thelibdirectory.
12-42: Test contract properly structured for verification flow testing.The contract has been renamed to
TreeNftVerificationTestand includes appropriate test fixtures with public addresses for planter and verifiers, theVerificationRemovedevent declaration for testing, and well-organized test constants. This provides a solid foundation for comprehensive verification testing.
44-90: Core verification flow tests are well-implemented.The tests properly validate:
- Basic verification with token minting (lines 44-60)
- Self-verification prevention (lines 62-73)
- Double verification prevention (lines 75-89)
All tests correctly use
vm.prank, expect appropriate reverts, and verify state changes.
91-174: Multi-verifier and removal tests provide comprehensive coverage.The test suite thoroughly validates:
- Multiple verifiers can verify the same tree (lines 91-115)
- Verification removal cleans up state correctly (lines 117-140)
- Removal properly updates verification counts and arrays (lines 142-174)
These tests ensure the verification system maintains consistency across complex scenarios.
176-290: Edge case and security tests are thorough.The test coverage includes:
- Token burning on verification removal (lines 176-195)
- Array cleanup when removing verifications (lines 197-222)
- Handling non-existent verification removal (lines 224-233)
- Event emission verification (lines 235-249)
- Insufficient token balance scenarios (lines 251-274)
- Ownership requirements for removal (lines 276-290)
This comprehensive edge case testing ensures robust functionality.
292-486: Query and state management tests validate data integrity.The remaining tests properly validate:
- Verifier retrieval functions (lines 292-337)
- Revocation counter increments (lines 339-358)
- Invalid tree ID handling (lines 360-366)
- PlanterToken lifecycle (lines 368-384)
- Complex multi-tree scenarios (lines 386-486)
These tests ensure data structures remain consistent through various operations.
src/TreeNft.sol (4)
45-57: Constructor signature updated to accept only core tokens.The constructor now takes only
_careTokenContractand_legacyTokenContractaddresses, removing dependencies on VerifierToken and PlanterToken. PlanterTokens are now created dynamically per verifier during the first verification.
61-101: mintNft extended with metadata and numberOfTrees parameters.The function now accepts
metadataandnumberOfTreesparameters, which are stored in the Tree struct. ThenumberOfTreesfield is later used to determine how many planter tokens to mint during verification.Ensure that
numberOfTreesis properly validated in upstream code or UI to prevent minting excessive tokens. Consider adding a reasonable upper bound check to prevent abuse.
427-443: getUserProfile function properly retrieves user details.The function validates user existence and returns comprehensive user details including token balances. The implementation correctly maps stored User data to the UserDetails return struct.
241-297: removeVerification now correctly clears the verification flag
The mappings_tokenIDtoUserVerification[tokenId][verifier]is set tofalseat line 251, soisVerified(tokenId, verifier)will returnfalseafter removal.
IronJam11
force-pushed
the
minorBugFixes
branch
3 times, most recently
from
0425b0b to
9c45baf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/TreeNft.sol (1)
207-239: Ensure consistent decimal precision in burn operation. In src/TreeNft.sol line 281,burnFromis called withtree.numberOfTrees(missing* 1e18), causing a units mismatch. Update toplanterToken.burnFrom(treeOwner, tree.numberOfTrees * 1e18).
♻️ Duplicate comments (1)
src/TreeNft.sol (1)
445-465: Critical: Fix uninitialized array in getUserVerifierTokenDetails.The function writes to
verifierTokenDetails[i]without initializing the array, which will cause every call to revert. This matches the issue flagged in past review comments.Apply this diff to initialize the array:
function getUserVerifierTokenDetails(address userAddress) public view returns (VerificationDetails[] memory verifierTokenDetails) { // This function returns the verifier token address of the user TreeNftVerification[] memory userVerifications = s_userToVerifications[userAddress]; + verifierTokenDetails = new VerificationDetails[](userVerifications.length); for (uint256 i = 0; i < userVerifications.length; i++) { PlanterToken planterToken = PlanterToken(userVerifications[i].verifierPlanterTokenAddress); verifierTokenDetails[i] = VerificationDetails({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/test.yml(2 hunks)src/Organisation.sol(4 hunks)src/OrganisationFactory.sol(2 hunks)src/TreeNft.sol(8 hunks)src/utils/structs.sol(4 hunks)test/Organisation.t.sol(8 hunks)test/OrganisationFactory.t.sol(6 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/utils/structs.sol
🔇 Additional comments (17)
.github/workflows/test.yml (1)
10-10: Stable toolchain switch looks good.Disabling the nightly warning and pinning to the stable toolchain keeps CI noise-free while tracking the latest stable forge release. 👍
Also applies to: 27-27
src/OrganisationFactory.sol (1)
113-113: LGTM! Field rename is consistent.The rename from
photoIpfsHashtoorganisationPhotoaligns with the updatedOrganisationDetailsstruct definition and maintains consistency across success and error paths.Also applies to: 126-126
test/Organisation.t.sol (2)
155-157: LGTM! mintNft calls updated with new parameters.The test calls to
mintNftnow correctly includeQR_HASH,METADATA, andNUMBER_OF_TREESparameters, aligning with the extended function signature.Also applies to: 189-191, 271-273
240-242: LGTM! plantTreeProposal calls updated with new parameters.The test calls to
plantTreeProposalnow correctly includeMETADATAandNUMBER_OF_TREESparameters, matching the extended function signature.Also applies to: 295-297
src/utils/structs.sol (4)
8-8: LGTM! Field renames improve clarity.The renaming of photo-related fields (
photoIpfsHash→organisationPhoto,profilePhotoIpfs→profilePhoto,qrIpfsHash→qrPhoto) makes the codebase more flexible and not tied to IPFS storage, allowing for different storage solutions.Also applies to: 50-50, 76-76, 92-92
27-35: LGTM! New VerificationDetails struct supports enhanced verification flow.The new
VerificationDetailsstruct appropriately captures verification metadata including the verifier's planter token address and number of trees, supporting the per-user token management feature.
57-66: LGTM! UserDetails struct provides comprehensive user information.The new
UserDetailsstruct consolidates user profile data with token balances, providing a complete view of user information in a single query.
69-69: LGTM! Tree struct enhancements support expanded metadata.The additions of
id,metadata, andnumberOfTreesfields to theTreestruct enable richer tree data tracking and align with the enhanced minting flow.Also applies to: 77-77, 83-83
src/Organisation.sol (3)
260-264: LGTM! Function signature extended to support enhanced tree metadata.The
plantTreeProposalfunction now correctly accepts_qrPhoto,_metadata, andnumberOfTreesparameters, aligning with the enhanced tree data model.
276-281: LGTM! Proposal initialization updated with new fields.The
TreePlantingProposalstruct is properly initialized with the newqrPhoto,metadata, andnumberOfTreesfields.
287-297: LGTM! mintNft calls consistently updated.Both calls to
mintNft(on proposal creation with ≤2 owners and on voting approval) correctly pass all new parameters includingqrPhoto,metadata, andnumberOfTrees.Also applies to: 401-411
src/TreeNft.sol (6)
45-57: LGTM! Constructor simplified to remove token contract dependencies.The TreeNft constructor now only accepts
careTokenandlegacyTokenaddresses, removing the dependency on pre-deployed PlanterToken and VerifierToken contracts. This aligns with the new per-user planter token creation pattern.
61-101: LGTM! mintNft function extended with enhanced metadata.The function signature and Tree struct initialization correctly incorporate the new
qrPhoto,metadata, andnumberOfTreesparameters. The tree ID is properly set froms_treeTokenCounterbefore incrementing.
213-213: LGTM! Self-verification check prevents gaming the system.The addition of
if (msg.sender == treeOwner) revert CannotVerifyOwnTree();prevents tree owners from verifying their own trees, which is essential to maintain verification integrity.
218-223: LGTM! Per-user planter token pattern improves scalability.Creating a unique PlanterToken contract per verifier (lines 218-223) allows independent token management and prevents conflicts. The pattern of checking if a user's planter token exists before creating a new one is correct.
427-443: LGTM! getUserProfile provides comprehensive user information.The function correctly retrieves user details from storage and enriches them with token balances from the CareToken and LegacyToken contracts.
410-410: LGTM! Legacy token minting uses correct decimal precision.Line 410 correctly mints legacy tokens with
1 * 1e18to account for 18 decimals.
| treeNft = new TreeNft(address(careToken), address(legacyToken)); | ||
|
|
||
| careToken.transferOwnership(address(treeNft)); | ||
| planterToken.transferOwnership(address(treeNft)); | ||
| verifierToken.transferOwnership(address(treeNft)); | ||
| legacyToken.transferOwnership(address(treeNft)); |
There was a problem hiding this comment.
Remove obsolete PlanterToken ownership transfer.
The TreeNft constructor no longer accepts planterToken as a parameter (line 52), but line 55 still attempts to transfer its ownership to TreeNft. This call will succeed but serves no purpose since TreeNft doesn't manage this token anymore.
Apply this diff to remove the obsolete ownership transfer:
careToken = new CareToken(owner);
planterToken = new PlanterToken(owner);
legacyToken = new LegacyToken(owner);
treeNft = new TreeNft(address(careToken), address(legacyToken));
careToken.transferOwnership(address(treeNft));
- planterToken.transferOwnership(address(treeNft));
legacyToken.transferOwnership(address(treeNft));📝 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.
| treeNft = new TreeNft(address(careToken), address(legacyToken)); | |
| careToken.transferOwnership(address(treeNft)); | |
| planterToken.transferOwnership(address(treeNft)); | |
| verifierToken.transferOwnership(address(treeNft)); | |
| legacyToken.transferOwnership(address(treeNft)); | |
| careToken = new CareToken(owner); | |
| planterToken = new PlanterToken(owner); | |
| legacyToken = new LegacyToken(owner); | |
| treeNft = new TreeNft(address(careToken), address(legacyToken)); | |
| careToken.transferOwnership(address(treeNft)); | |
| legacyToken.transferOwnership(address(treeNft)); |
🤖 Prompt for AI Agents
In test/Organisation.t.sol around lines 52 to 56, the test still transfers
ownership of planterToken to TreeNft even though the TreeNft constructor no
longer accepts or manages planterToken; remove the obsolete
planterToken.transferOwnership(address(treeNft)); call so only relevant tokens
(careToken and legacyToken) have their ownership transferred to the TreeNft
instance.
| legacyToken = new LegacyToken(owner); | ||
|
|
||
| treeNft = new TreeNft(address(careToken), address(planterToken), address(verifierToken), address(legacyToken)); | ||
| treeNft = new TreeNft(address(careToken), address(legacyToken)); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify TreeNft contract interaction with CareToken and LegacyToken.
The TreeNft constructor now only accepts careToken and legacyToken addresses. Ensure that all TreeNft functionality that previously relied on PlanterToken or VerifierToken has been properly refactored.
Run the following script to verify TreeNft no longer references the removed token contracts:
🏁 Script executed:
#!/bin/bash
# Description: Verify that TreeNft.sol does not reference VerifierToken
# Check for any remaining VerifierToken references
rg -n "VerifierToken" --type sol --glob '!*test*'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Description: Verify that no Solidity contracts reference VerifierToken or PlanterToken
rg -nP "(VerifierToken|PlanterToken)" -g '*.sol' --glob '!*test*'Length of output: 1983
Refactor TreeNft.sol to remove PlanterToken/VerifierToken references.
TreeNft.sol still imports PlanterToken (line 17) and retains mappings and logic for planter/verifier tokens (lines 34–35, 218–223, 236, 276–282, 445–462). Eliminate or update these remnants to match the new constructor signature.
🤖 Prompt for AI Agents
In test/OrganisationFactory.t.sol around line 42, TreeNft is being instantiated
using the old constructor that expects PlanterToken/VerifierToken; refactor
TreeNft.sol to remove the PlanterToken import and all planter/verifier-related
state and logic (mappings at lines ~34–35, functions and logic at ~218–223, 236,
276–282, 445–462) so the contract matches the new constructor signature, then
update the test to instantiate TreeNft with the new constructor parameters
(remove passing PlanterToken/VerifierToken addresses and any tests that rely on
the removed mappings or functions). Ensure all imports, state variables,
modifiers, and tests referencing PlanterToken/VerifierToken are deleted or
adapted to the new design.
9c45baf to
f0813c4
Compare
IronJam11
force-pushed
the
minorBugFixes
branch
from
f0813c4 to
1e7d659
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/structs.sol (1)
27-35: Consider documenting the distinction between VerificationDetails and TreeNftVerification.Both structs share similar fields but serve different purposes:
VerificationDetailsincludesnumberOfTrees(aggregate verification data)TreeNftVerificationincludestreeNftId(per-NFT verification data)To reduce confusion, consider adding inline comments explaining when each struct should be used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Organisation.sol(4 hunks)src/utils/structs.sol(4 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/utils/structs.sol
🔇 Additional comments (10)
src/utils/structs.sol (7)
8-8: LGTM: Consistent naming improvement.The field rename from
photoIpfsHashtoorganisationPhotoimproves consistency with other photo field names in the codebase.
24-24: LGTM: Tracking verifier planter token address.The addition of
verifierPlanterTokenAddressenables linking verifications to specific planter tokens, which aligns with the per-user PlanterToken architecture mentioned in the PR objectives.
50-50: LGTM: Consistent naming improvement.The field rename from
profilePhotoIpfstoprofilePhotoaligns with the naming convention used inorganisationPhoto.
57-66: LGTM: Extended user profile for frontend.The new
UserDetailsstruct appropriately extendsUserwith token balance fields (legacyTokensandcareTokens) for frontend display purposes, as mentioned in the PR objectives.
76-77: LGTM: Enhanced tree metadata and batch support.The additions of
qrPhoto(renamed fromqrIpfsHash),metadata, andnumberOfTreesfields enable:
- Consistent photo field naming
- Rich metadata storage for NFTs
- Support for batch tree planting scenarios
These changes align well with the PR objectives to support frontend use cases and reduce gas fees.
Also applies to: 83-83
92-92: LGTM: Proposal struct enhanced with metadata and initiator tracking.The changes properly extend
TreePlantingProposalwith:
qrPhoto(renamed fromqrIpfsHash) for consistencymetadatafor rich proposal contextnumberOfTreesfor batch proposalsinitiatorfor tracking proposal originAll fields are correctly initialized in Organisation.sol (lines 270-283).
Also applies to: 95-95, 97-98
69-69: Tree.id is initialized correctly
mintNftpassestokenIdas the first argument toTree, so theidfield is properly set.src/Organisation.sol (3)
260-264: LGTM: Function signature properly updated.The
plantTreeProposalsignature correctly incorporates:
_qrPhoto(replacing_qrIpfshash) for consistent naming_metadatafor rich proposal contextnumberOfTreesfor batch planting supportAll parameters have proper
memoryqualifiers.
270-283: LGTM: All new struct fields properly initialized.The
TreePlantingProposalconstruction correctly initializes:
qrPhotowith_qrPhotoparametermetadatawith_metadataparameterstatuswith 0 (pending state)numberOfTreeswith function parameterinitiatorwithmsg.senderfor proper tracking
288-298: TreeNft.mintNft signature matches updated parameters; no changes required.
| if (planterToken.balanceOf(treeOwner) >= tokensToReturn) { | ||
| planterToken.burnFrom(treeOwner, tokensToReturn); | ||
| address[] storage verifierTokenAddrs = s_userToVerifierTokenAddresses[treeOwner]; | ||
| for (uint256 j = 0; j < verifierTokenAddrs.length; j++) { |
There was a problem hiding this comment.
Could this for loop become too expensive?
|
|
||
| TreeNftVerification[] memory userVerifications = s_userToVerifications[userAddress]; | ||
| for(uint i = 0; i < userVerifications.length; i++) { | ||
| for (uint256 i = 0; i < userVerifications.length; i++) { |
There was a problem hiding this comment.
Could this for loop become too expensive?
There was a problem hiding this comment.
you are right, i will write a paginated view function.
| uint256 private s_deathCounter; | ||
| uint256 private s_treeNftVerification; | ||
| uint256 private s_userCounter; | ||
|
|
There was a problem hiding this comment.
Are some of the imports above still used?
There was a problem hiding this comment.
yes sir - for mappings majorly
src/utils/structs.sol
Outdated
| string description; | ||
| bool isHidden; | ||
| uint256 treeNftId; | ||
| address verifierPlanterTokenAddress; |
There was a problem hiding this comment.
What is this address?
There was a problem hiding this comment.
the unique planter token contract address of very every planter
| address[] ancestors; | ||
| uint256 lastCareTimestamp; | ||
| uint256 careCount; | ||
| uint256 numberOfTrees; |
There was a problem hiding this comment.
memtioned on discord
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores
Bug Fixes