Merged
Conversation
📝 WalkthroughWalkthroughAdded Tron-specific implementations for Catapultar clone factory and cloning utilities, including a deterministic clone library ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CatapultarFactoryTron
participant LibCloneTron
participant Catapultar as Catapultar (Proxy)
Caller->>CatapultarFactoryTron: deploy(ktp, owner, salt)
CatapultarFactoryTron->>CatapultarFactoryTron: ownerInSalt validation
CatapultarFactoryTron->>LibCloneTron: cloneDeterministic_PUSH0(EXECUTOR, salt)
LibCloneTron->>LibCloneTron: predictDeterministicAddress_PUSH0
LibCloneTron->>Catapultar: CREATE2 (deterministic)
Catapultar-->>LibCloneTron: address (created)
LibCloneTron-->>CatapultarFactoryTron: instance address
CatapultarFactoryTron->>Catapultar: init(ktp, owner, msg.value)
Catapultar-->>Caller: proxy address
sequenceDiagram
participant Caller
participant CatapultarFactoryTron
participant EfficientHashLib
participant LibCloneTron
participant Catapultar as Catapultar (Proxy)
Caller->>CatapultarFactoryTron: deployWithDigest(ktp, owner, salt, digest, isSignature)
CatapultarFactoryTron->>CatapultarFactoryTron: ownerInSalt validation
CatapultarFactoryTron->>EfficientHashLib: hash(salt, digest, nonce)
EfficientHashLib-->>CatapultarFactoryTron: derived_salt
CatapultarFactoryTron->>LibCloneTron: cloneDeterministic_PUSH0(EXECUTOR, derived_salt)
LibCloneTron->>Catapultar: CREATE2
Catapultar-->>LibCloneTron: address (created)
LibCloneTron-->>CatapultarFactoryTron: instance address
CatapultarFactoryTron->>Catapultar: setSignature(digest, DigestApproval)
CatapultarFactoryTron->>Catapultar: init(ktp, owner, msg.value)
Catapultar-->>Caller: proxy address
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
solidity/src/libs/LibClone.tron.sol (1)
89-92: Remove unnecessaryreturnkeyword.
LibClone.checkStartsWithhas no return value (it reverts on failure). Thereturnkeyword is syntactically valid but misleading and unnecessary.Suggested fix
function checkStartsWith(bytes32 salt, address by) internal pure { - return LibClone.checkStartsWith(salt, by); + LibClone.checkStartsWith(salt, by); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/src/libs/LibClone.tron.sol` around lines 89 - 92, The function checkStartsWith currently uses a misleading "return" when calling LibClone.checkStartsWith(salt, by) even though that callee is non-returning (reverts on failure); remove the unnecessary "return" keyword and call LibClone.checkStartsWith(salt, by); directly so the function remains a void/internal call (keep parameter names salt and by and the function signature unchanged).solidity/test/Tron/CatapultarFactory.tron.t.sol (1)
172-184: Salt validation fuzz tests may have unexpected pass/fail cases.The test logic sets
vm.expectRevertconditionally based on the salt prefix. However, when the condition is false (salt starts with0x00...or matches owner), the test proceeds to call the function without revert expectation.For
deployspecifically, if salt is valid but the deployment has already occurred (e.g., from a previous fuzz run with same salt), the call would revert withDeploymentFailed()instead, potentially causing unexpected test failures.Consider using
vm.assumeto filter out salts that would cause deployment collisions, or usetry/catchto distinguish expected reverts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/test/Tron/CatapultarFactory.tron.t.sol` around lines 172 - 184, The fuzz test testRevert_deploy_salt_does_not_contain_owner can spuriously fail when a valid salt collides with a previous deployment (causing DeploymentFailed) because you only set vm.expectRevert conditionally; update the test to filter out collision salts (use vm.assume to exclude salts whose computed address has already been deployed) or wrap the factory.deploy call in a try/catch and assert the revert reason is either SaltDoesNotStartWith() or DeploymentFailed(), referencing the existing test function name testRevert_deploy_salt_does_not_contain_owner and the factory.deploy call and the SaltDoesNotStartWith/DeploymentFailed revert signatures so the logic distinguishes salt-validation failures from deployment-collision failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@solidity/src/CatapultarFactory.tron.sol`:
- Around line 6-8: The import statements (LibCloneTron, Catapultar,
KeyedOwnable) are misformatted causing forge fmt --check to fail; fix by running
formatter (forge fmt) or manually reorder/adjust spacing so imports follow
standard Solidity/forge formatting conventions (consistent spacing, no extra
blank lines, correct alphabetical/order grouping) in the
CatapultarFactory.tron.sol imports section; ensure the final file passes forge
fmt --check.
In `@solidity/src/libs/LibClone.tron.sol`:
- Around line 28-93: CI flagged formatting issues in LibCloneTron — run forge
fmt and fix spacing/comment alignment and function signature breaks so the file
passes `forge fmt --check`; specifically normalize comment spacing and line
breaks around function declarations like predictDeterministicAddress,
cloneDeterministic_PUSH0, predictDeterministicAddress_PUSH0,
deployDeterministicERC1967, predictDeterministicAddressERC1967, and
checkStartsWith to match project style (wrap long parameter lists consistently,
ensure doc-comments have a single space after triple slashes, and align
braces/indentation) then re-run `forge fmt` to verify.
In `@solidity/test/Tron/Catapultar/Catapultar.minimal.tron.t.sol`:
- Around line 4-8: The import ordering in Catapultar.minimal.tron.t.sol is
failing CI's `forge fmt --check`; fix by reordering the import statements (or
run `forge fmt`) so they comply with Forge's formatting rules—e.g., ensure
imports are grouped and sorted (place "../../../src/libs/LibClone.tron.sol",
then "../../Catapultar/Catapultar.base.t.sol", then
"../../mocks/MockCatapultar.sol" or simply run `forge fmt` to apply the correct
ordering for the imports referencing LibCloneTron, CatapultarTest, and
MockCatapultar.
In `@solidity/test/Tron/Catapultar/Catapultar.upgradable.tron.t.sol`:
- Around line 4-8: CI failed due to import ordering in the test file; run `forge
fmt` to auto-fix or manually reorder the import statements so they follow the
project's formatting rules (group and/or alphabetize imports): place the
LibCloneTron import (LibCloneTron) before the mock and test imports, then
MockCatapultar, then CatapultarTest, and re-run `forge fmt --check` to verify
resolution.
In `@solidity/test/Tron/CatapultarFactory.tron.t.sol`:
- Around line 97-169: CI flagged formatting issues in the test functions (e.g.,
test_predictDeploy_different_salts, test_predictDeployWithDigest_deterministic,
test_predictDeployWithDigest_digest_affects_address,
test_predictDeployUpgradeable_deterministic,
test_predictDeployUpgradeable_differs_from_minimal): run the formatter and fix
signature/line-wrapping so the functions and long factory calls conform to the
project's style; specifically run `forge fmt` (or apply the same formatting
rules) to reflow long argument lists for factory.predictDeployWithDigest,
factory.predictDeployUpgradeable, and ensure function declarations use the
canonical spacing/line breaks, then commit the formatted changes.
---
Nitpick comments:
In `@solidity/src/libs/LibClone.tron.sol`:
- Around line 89-92: The function checkStartsWith currently uses a misleading
"return" when calling LibClone.checkStartsWith(salt, by) even though that callee
is non-returning (reverts on failure); remove the unnecessary "return" keyword
and call LibClone.checkStartsWith(salt, by); directly so the function remains a
void/internal call (keep parameter names salt and by and the function signature
unchanged).
In `@solidity/test/Tron/CatapultarFactory.tron.t.sol`:
- Around line 172-184: The fuzz test
testRevert_deploy_salt_does_not_contain_owner can spuriously fail when a valid
salt collides with a previous deployment (causing DeploymentFailed) because you
only set vm.expectRevert conditionally; update the test to filter out collision
salts (use vm.assume to exclude salts whose computed address has already been
deployed) or wrap the factory.deploy call in a try/catch and assert the revert
reason is either SaltDoesNotStartWith() or DeploymentFailed(), referencing the
existing test function name testRevert_deploy_salt_does_not_contain_owner and
the factory.deploy call and the SaltDoesNotStartWith/DeploymentFailed revert
signatures so the logic distinguishes salt-validation failures from
deployment-collision failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1754c03b-3180-4007-b5c7-ba90111bb873
📒 Files selected for processing (8)
solidity/snapshots/CatapultarFactoryTronTest.jsonsolidity/snapshots/CatapultarMinimalTronTest.jsonsolidity/snapshots/CatapultarUpgradeableTronTest.jsonsolidity/src/CatapultarFactory.tron.solsolidity/src/libs/LibClone.tron.solsolidity/test/Tron/Catapultar/Catapultar.minimal.tron.t.solsolidity/test/Tron/Catapultar/Catapultar.upgradable.tron.t.solsolidity/test/Tron/CatapultarFactory.tron.t.sol
Comment on lines
+4
to
+8
| import { LibCloneTron } from "../../../src/libs/LibClone.tron.sol"; | ||
|
|
||
| import { MockCatapultar } from "../../mocks/MockCatapultar.sol"; | ||
| import { CatapultarTest } from "../../Catapultar/Catapultar.base.t.sol"; | ||
|
|
There was a problem hiding this comment.
Fix import ordering flagged by CI.
The pipeline reports forge fmt --check failures for import ordering. Run forge fmt to resolve.
🧰 Tools
🪛 GitHub Actions: Forge Test & Coverage
[error] 6-8: forge fmt --check failed due to formatting diffs in import ordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@solidity/test/Tron/Catapultar/Catapultar.minimal.tron.t.sol` around lines 4 -
8, The import ordering in Catapultar.minimal.tron.t.sol is failing CI's `forge
fmt --check`; fix by reordering the import statements (or run `forge fmt`) so
they comply with Forge's formatting rules—e.g., ensure imports are grouped and
sorted (place "../../../src/libs/LibClone.tron.sol", then
"../../Catapultar/Catapultar.base.t.sol", then "../../mocks/MockCatapultar.sol"
or simply run `forge fmt` to apply the correct ordering for the imports
referencing LibCloneTron, CatapultarTest, and MockCatapultar.
Comment on lines
+4
to
+8
| import { LibCloneTron } from "../../../src/libs/LibClone.tron.sol"; | ||
|
|
||
| import { MockCatapultar } from "../../mocks/MockCatapultar.sol"; | ||
| import { CatapultarTest } from "../../Catapultar/Catapultar.base.t.sol"; | ||
|
|
There was a problem hiding this comment.
Fix import ordering flagged by CI.
The pipeline reports forge fmt --check failures for import ordering. Run forge fmt to resolve.
🧰 Tools
🪛 GitHub Actions: Forge Test & Coverage
[error] 6-8: forge fmt --check failed due to formatting diffs in import ordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@solidity/test/Tron/Catapultar/Catapultar.upgradable.tron.t.sol` around lines
4 - 8, CI failed due to import ordering in the test file; run `forge fmt` to
auto-fix or manually reorder the import statements so they follow the project's
formatting rules (group and/or alphabetize imports): place the LibCloneTron
import (LibCloneTron) before the mock and test imports, then MockCatapultar,
then CatapultarTest, and re-run `forge fmt --check` to verify resolution.
mirooon
approved these changes
Apr 8, 2026
|
lgtm, only gh action is failing. Please run |
This was referenced Apr 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Create a custom library for Tron contracts that uses 0x41 as the create2 prefix instead of 0xff.
Summary by CodeRabbit
New Features
Tests