[IP-527] Bring your own IPT: Tokenizer allows attaching arbitrary ERC20s #173
Conversation
WalkthroughAdds IIPToken and WrappedIPToken, migrates IPToken to implement IIPToken, extends Tokenizer to attach existing ERC‑20s and support dual reinit, updates Permissioner signatures to use IIPToken, introduces IPTokenUtils.generateURI, adds v14 rollout and tests, removes some v13 scripts/tests, and updates CI/docs and Foundry config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Tokenizer
participant Permissioner
participant ERC20 as ERC20 (existing)
participant WIPT as WrappedIPToken (clone)
User->>Tokenizer: attachIpt(ipnftId, agreementCid, signedAgreement, ERC20)
Tokenizer->>Tokenizer: _validateTokenContract(ERC20)\n(check non-zero, has code, decimals 0..18)
Tokenizer->>Permissioner: accept(IIPToken placeholder, User, signedAgreement)
Note over Tokenizer,Permissioner: Permissioner verifies signed terms
Tokenizer->>WIPT: clone + initialize(ipnftId, User, agreementCid, ERC20)
Tokenizer->>Tokenizer: synthesized[ipnftId] = WIPT
Tokenizer-->>User: return WIPT (IIPToken)
Note over Tokenizer: Emit TokensCreated, TokenWrapped
sequenceDiagram
autonumber
actor Admin
participant Proxy as Tokenizer Proxy
participant Impl as Tokenizer V14
participant IPT as IPToken Impl
participant WIPT as WrappedIPToken Impl
Admin->>Impl: deploy Tokenizer V14
Admin->>IPT: deploy IPToken implementation
Admin->>WIPT: deploy WrappedIPToken implementation
Admin->>Proxy: upgradeToAndCall(Impl, reinit(WIPT, IPT))
Proxy->>Impl: reinit(WIPT, IPT) [reinitializer]
Impl->>Impl: setWrappedIPTokenImplementation(WIPT)\nsetIPTokenImplementation(IPT)
Note over Proxy,Impl: Tokenizer configured with both implementations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
elmariachi111
changed the title
0xmme
force-pushed
the
feat/bring-your-own-ipt
branch
from
f3fba19 to
f3ba328
Compare
|
@coderabbitai full review |
0xmme
changed the title
✅ Actions performedReview triggered.
|
✅ Actions performedFull review 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SalesShareDistributor.sol (1)
48-50: Initializer missing for upgradeable contract (critical).
Upgradeables must call initializer hooks; owner is never set and anyone can call initialize repeatedly.Apply:
- function initialize(SchmackoSwap _schmackoSwap) public { - schmackoSwap = _schmackoSwap; - } + function initialize(SchmackoSwap _schmackoSwap) public initializer { + __UUPSUpgradeable_init(); + __OwnableUpgradeable_init(); + __ReentrancyGuardUpgradeable_init(); + schmackoSwap = _schmackoSwap; + }
🧹 Nitpick comments (27)
.github/workflows/test.yml (1)
28-33: Avoid TODO drift; add a non-blocking size report step.
If code size matters, re-enable reporting without failing CI.Apply as a separate step:
+ - name: Report bytecode sizes (non-blocking) + run: forge build --sizes || truesrc/SalesShareDistributor.sol (1)
137-142: NatSpec type mismatch for tokenContract.
Doc says IIPToken but the signature takes IPToken.Fix the comment:
- * @param tokenContract IIPToken the IPToken token contract + * @param tokenContract IPToken the IPToken token contractAlternatively, change the parameter type to IIPToken if intended.
README.md (1)
103-109: Clarify upgrade example arguments.
Make explicit that bytes encodesreinit(address wrapped,address iptoken).Suggest:
-// 0xTokenizer (address, bytes)(0xNewImpl, 0xNewWrappedIPTokenImpl 0xNewIPTokenImpl) +// upgradeToAndCall(newTokenizerImpl, abi.encodeWithSelector(Tokenizer.reinit.selector, newWrappedIPTImpl, newIPTImpl))Also ensure the example byte string matches
Tokenizer.reinit(address,address).src/IIPToken.sol (1)
1-2: Align pragma with the rest of the repo.Most files use 0.8.18; consider pinning here for consistency.
-pragma solidity ^0.8; +pragma solidity ^0.8.18;test/Forking/Tokenizer14UpgradeForkTest.t.sol (4)
5-5: Remove unused console import.Keeps tests tidy and avoids linter noise.
-import { console } from 'forge-std/console.sol';
23-24: Likely unused SafeERC20Upgradeable extension in this file.If not calling safe* wrappers here, drop the using to reduce confusion.
- using SafeERC20Upgradeable for IPToken;
35-43: Gitleaks “Generic API Key” is a false positive.These are public mainnet addresses for a fork test. Consider adding a test‑only allowlist to hush future scans.
Example gitleaks allowlist (repo root):
title = "allowlist test addresses" [[rules.allowlist.regexes]] description = "evm addresses in tests" regex = '''0x[a-fA-F0-9]{40}''' path = '''test/.*\.t\.sol'''
524-529: Make revert expectations explicit (optional).If WrappedIPToken uses a specific custom error, assert its selector to harden the test.
- vm.expectRevert(); // WrappedIPToken should not allow issue/cap operations + vm.expectRevert(<CustomError>.selector); // if defined WrappedIPToken(address(wrappedToken)).issue(alice, 1000 ether); - vm.expectRevert(); // WrappedIPToken should not allow cap operations + vm.expectRevert(<CustomError>.selector); // if defined WrappedIPToken(address(wrappedToken)).cap();src/Permissioner.sol (4)
12-20: Docs still say IPToken; update to IIPToken.Avoids confusion in generated docs.
- * @param tokenContract IPToken + * @param tokenContract IIPToken
23-26: BlindPermissioner.accept can be marked pure.No state reads/writes; slight gas win and clarity.
- function accept(IIPToken tokenContract, address _for, bytes calldata data) external { + function accept(IIPToken /*tokenContract*/, address /*_for*/, bytes calldata /*data*/) external pure {
55-61: Docs: adjust param type mention.Mirror IIPToken in natspec.
- * @param tokenContract IPToken + * @param tokenContract IIPToken
81-84: Docs: adjust param type mention here too.Keep natspec consistent.
- * @param tokenContract IPToken + * @param tokenContract IIPTokensrc/WrappedIPToken.sol (4)
16-21: Harden the cloneable implementation: disable initializers on the implementation contract.Prevents anyone from initializing the implementation instance.
contract WrappedIPToken is IIPToken, Initializable { IERC20Metadata public wrappedToken; Metadata internal _metadata; + constructor() { + _disableInitializers(); + }
97-103: Unify external_url with IPToken (use molecule.xyz).IPToken uses https://molecule.xyz while this uses https://molecule.to. Align for consistency.
- '","description": "IP Tokens, derived from IP-NFTs, are ERC-20 tokens governing IP pools.","decimals": ', + '","description": "IP Tokens, derived from IP-NFTs, are ERC-20 tokens governing IP pools.","decimals": ', Strings.toString(wrappedToken.decimals()), - ',"external_url": "https://molecule.to","image": "",', + ',"external_url": "https://molecule.xyz","image": "",',
86-89: Use totalIssued() for the “supply” field to keep semantics consistent.This reduces duplicate logic and keeps URI in sync if you later tweak supply semantics.
- '","supply": "', - Strings.toString(wrappedToken.totalSupply()), + '","supply": "', + Strings.toString(totalIssued()),
74-108: Optional: make uri() resilient to non‑standard ERC20 metadata.Some ERC20s poorly implement name/symbol/decimals and can revert. Consider try/catch with sane fallbacks to avoid uri() reverting.
test/TokenizerWrapped.t.sol (4)
79-83: Stop prank at end of setUp to avoid cross‑test leakage.ipnft.mintReservation{ value: MINTING_FEE }(originalOwner, reservationId, ipfsUri, DEFAULT_SYMBOL, ""); + vm.stopPrank();
120-134: Assert decimals passthrough as well.Adds a cheap check that the wrapper’s decimals mirror the underlying ERC20.
assertEq(tokenContract.name(), "TestToken"); assertEq(tokenContract.symbol(), "TEST"); + assertEq(tokenContract.decimals(), IERC20Metadata(address(erc20)).decimals());
98-109: Add a regression test for double‑attach (AlreadyTokenized).Covers reentrance/duplication attempts on the same ipnftId.
function testCannotAttachInvalidTokenContract() public { vm.startPrank(originalOwner); @@ tokenizer.attachIpt(1, agreementCid, "", IERC20Metadata(alice)); } + + function testCannotAttachTwice() public { + vm.startPrank(originalOwner); + erc20 = new FakeERC20("TT", "TT"); + erc20.mint(originalOwner, 1_000 ether); + tokenizer.attachIpt(1, agreementCid, "", erc20); + vm.expectRevert(AlreadyTokenized.selector); + tokenizer.attachIpt(1, agreementCid, "", erc20); + }
151-177: Optional: validate uri() JSON fields.Leverage contains() to assert erc20_contract and agreement_content are present with expected values.
src/IPToken.sol (1)
116-125: Keep external_url consistent across tokens.This uses molecule.xyz while WrappedIPToken used molecule.to. Align both (prefer molecule.xyz).
src/Tokenizer.sol (5)
159-163: Guard against unset ipTokenImplementation before clone.Fail fast with a clear error instead of bubbling a low‑level revert.
// https://github.com/OpenZeppelin/workshops/tree/master/02-contracts-clone + if (address(ipTokenImplementation) == address(0)) { + revert ZeroAddress(); + } token = IPToken(Clones.clone(address(ipTokenImplementation)));
204-207: Guard against unset wrappedTokenImplementation before clone.Same rationale as for ipTokenImplementation.
- WrappedIPToken wrappedIpt = WrappedIPToken(Clones.clone(address(wrappedTokenImplementation))); + if (address(wrappedTokenImplementation) == address(0)) { + revert ZeroAddress(); + } + WrappedIPToken wrappedIpt = WrappedIPToken(Clones.clone(address(wrappedTokenImplementation)));
190-193: Prefer calldata for agreementCid to save gas.- function attachIpt(uint256 ipnftId, string memory agreementCid, bytes calldata signedAgreement, IERC20Metadata tokenContract) + function attachIpt(uint256 ipnftId, string calldata agreementCid, bytes calldata signedAgreement, IERC20Metadata tokenContract)
208-219: Harden metadata reads in attachIpt to tolerate non‑standard ERC20s.Some tokens revert on name/symbol/totalSupply; use try/catch with fallbacks to avoid whole tx reverts.
- emit TokensCreated( + uint256 supply_; + string memory name_; + string memory symbol_; + try tokenContract.totalSupply() returns (uint256 s) { supply_ = s; } catch { supply_ = 0; } + try tokenContract.name() returns (string memory n) { name_ = n; } catch { name_ = ""; } + try tokenContract.symbol() returns (string memory sy) { symbol_ = sy; } catch { symbol_ = ""; } + + emit TokensCreated( uint256(keccak256(abi.encodePacked(ipnftId))), ipnftId, address(tokenContract), _msgSender(), - tokenContract.totalSupply(), + supply_, agreementCid, - tokenContract.name(), - tokenContract.symbol() + name_, + symbol_ );
128-134: Update NatSpec for reinit.Comment says “sets legacy IPTs on the tokenized mapping,” but now only sets implementation addresses.
src/helpers/test-upgrades/Tokenizer13.sol (1)
112-123: Explicitly document that hardcoded synthesized entries are test‑only.Prevents accidental reuse in non‑test deployments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
.github/workflows/test.yml(1 hunks)README.md(1 hunks)script/dev/Synthesizer.s.sol(0 hunks)script/dev/Tokenizer.s.sol(3 hunks)script/prod/RolloutTokenizerV13.s.sol(0 hunks)script/prod/RolloutTokenizerV14.s.sol(1 hunks)src/IIPToken.sol(1 hunks)src/IPToken.sol(5 hunks)src/Permissioner.sol(5 hunks)src/SalesShareDistributor.sol(2 hunks)src/Tokenizer.sol(7 hunks)src/WrappedIPToken.sol(1 hunks)src/helpers/test-upgrades/IPToken13.sol(1 hunks)src/helpers/test-upgrades/Tokenizer12.sol(0 hunks)src/helpers/test-upgrades/Tokenizer13.sol(1 hunks)test/CrowdSalePermissioned.t.sol(1 hunks)test/Forking/Tokenizer13UpgradeForkTest.t.sol(0 hunks)test/Forking/Tokenizer14UpgradeForkTest.t.sol(1 hunks)test/Mintpass.t.sol(0 hunks)test/Permissioner.t.sol(1 hunks)test/Tokenizer.t.sol(3 hunks)test/TokenizerWrapped.t.sol(1 hunks)test/helpers/Strings.sol(1 hunks)
💤 Files with no reviewable changes (5)
- test/Mintpass.t.sol
- test/Forking/Tokenizer13UpgradeForkTest.t.sol
- script/prod/RolloutTokenizerV13.s.sol
- script/dev/Synthesizer.s.sol
- src/helpers/test-upgrades/Tokenizer12.sol
🧰 Additional context used
🪛 Gitleaks (8.27.2)
test/Forking/Tokenizer14UpgradeForkTest.t.sol
[high] 36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (20)
test/helpers/Strings.sol (1)
7-7: LGTM — whitespace tweak only.
No behavioral change; safe.test/Permissioner.t.sol (1)
6-8: Imports switched to IIPToken.Metadata — OK.
Matches the interface split; no logic impact.src/SalesShareDistributor.sol (1)
11-13: Import split (Metadata from IIPToken) — OK.
Consistent with interface migration.test/CrowdSalePermissioned.t.sol (1)
11-12: Imports updated to IIPToken.Metadata — OK.
Keeps tests aligned with interface relocation.script/dev/Tokenizer.s.sol (1)
29-36: WrappedIPToken wiring added — looks good.
Deployment now sets both IPToken and WrappedIPToken implementations and logs both.script/prod/RolloutTokenizerV14.s.sol (1)
10-31: Rollout script generates reinit calldata for V1.4 — LGTM.
Clear outputs; no on-chain side effects.test/Tokenizer.t.sol (4)
20-22: Import switch to IIPToken.Metadata looks correct.Alias usage is clear and consistent with the new interface split.
26-28: OK to make controllerOf() pure.No state reads; this tightens the interface.
221-221: Dropping the unused return value is fine here.The test only needs the side‑effects of tokenizeIpnft().
299-318: Good coverage of post‑transfer issuance rules.Test correctly asserts the old owner is blocked and new owner can issue.
src/IIPToken.sol (1)
10-21: Interface surface LGTM.Minimal, stable, and matches downstream usage in Tokenizer/Permissioners.
src/Permissioner.sol (3)
7-8: Import switch to IIPToken/Metadata is correct.Matches the new common interface.
34-36: Event uses address — fine for interface‑agnostic logging.No change needed.
47-52: Signature path looks correct.Emits after verification; no on‑chain storage — good.
src/helpers/test-upgrades/IPToken13.sol (4)
27-45: Initializer and ownership model match legacy behavior._disableInitializers in ctor and explicit initialize() are correct for upgradeable pattern.
51-56: Access control matches 1.3 semantics.Tokenizer owner or IPNFT controller can act; good for migration tests.
67-74: Issue() updates totalIssued then mints — OK.Reverts on capped via custom error; aligns with tests.
87-119: URI assembly is fine for fork/migration tests.Fields align with expectations; no on‑chain string concatenation hotspots outside tests.
src/IPToken.sol (1)
23-41: LGTM on interface alignment and upgrade‑safety.Inherits IIPToken cleanly, adds constructor to disable initializers, and preserves storage layout.
src/helpers/test-upgrades/Tokenizer13.sol (1)
21-172: LGTM for upgrade path scaffolding.Storage/layout and cloning flow align with the main Tokenizer versioning.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/SalesShareDistributor.sol (2)
48-50: Initializer missing initializer guard and parent inits (upgradeable critical).
initializeis public withoutinitializerand doesn’t call OZ init routines; anyone can call it and owner isn’t set. Add_disableInitializersin constructor and proper init calls.-contract SalesShareDistributor is UUPSUpgradeable, OwnableUpgradeable, ReentrancyGuardUpgradeable { +contract SalesShareDistributor is UUPSUpgradeable, OwnableUpgradeable, ReentrancyGuardUpgradeable { + constructor() { + _disableInitializers(); + } @@ - function initialize(SchmackoSwap _schmackoSwap) public { - schmackoSwap = _schmackoSwap; - } + function initialize(SchmackoSwap _schmackoSwap) public initializer { + __UUPSUpgradeable_init(); + __OwnableUpgradeable_init(); + __ReentrancyGuardUpgradeable_init(); + schmackoSwap = _schmackoSwap; + }
175-181: Enforce capped invariant before activating sales (prevent dilution) — fix requiredThe safety check in _startClaimingPhase is commented out; re-enable a runtime guard so late issuance cannot dilute claims.
- src/SalesShareDistributor.sol (around _startClaimingPhase, ~lines 174–181): reinstate a check before setting sales and emitting SalesActivated, e.g. use the existing UncappedToken() error: if (!ipt.capped()) revert UncappedToken();
- src/IPToken.sol: expose a public accessor for the capped state if one does not exist (e.g. function capped() public view returns (bool) or a public bool set when cap() is called) so the distributor can compile and reliably enforce the invariant.
🧹 Nitpick comments (27)
test/helpers/Strings.sol (2)
5-11: Add explicit bounds checks and clearer indexing.Current code relies on implicit OOB reverts and redoes subtraction each loop. Add requires with reasons and iterate over a computed length.
Apply this diff:
function substring(string memory str, uint256 startIndex, uint256 endIndex) internal pure returns (string memory) { - bytes memory strBytes = bytes(str); - bytes memory result = new bytes(endIndex - startIndex); - for (uint256 i = startIndex; i < endIndex; i++) { - result[i - startIndex] = strBytes[i]; - } + bytes memory strBytes = bytes(str); + require(endIndex >= startIndex, "Strings: end < start"); + require(endIndex <= strBytes.length, "Strings: end > length"); + uint256 len = endIndex - startIndex; + bytes memory result = new bytes(len); + for (uint256 j = 0; j < len; j++) { + result[j] = strBytes[startIndex + j]; + } return string(result); }Also confirm callers pass byte indices (not Unicode code points), as this slices raw UTF‑8 bytes.
4-4: Avoid name collision with OpenZeppelin’s Strings.If OZ’s Strings is imported elsewhere, this local test helper can collide in
using Strings for .... Consider renaming to TestStrings or aliasing the import at call sites..github/workflows/test.yml (1)
28-33: Reintroduce contract size reporting (non-blocking).Dropping --sizes removes an early signal for bytecode growth. Keep build fast, but add a separate, non-failing sizes step.
- name: Run Forge build run: | forge --version - forge build + forge build id: build + - name: Report contract sizes (non-blocking) + if: always() + run: | + set -eo pipefail + forge build --sizes || truesrc/IIPToken.sol (2)
2-2: Align pragma with the repo (pin to ^0.8.18).Other files use ^0.8.18/0.8.18. Unify to avoid mixed compiler behavior.
-pragma solidity ^0.8; +pragma solidity ^0.8.18;
10-21: Consider future-proofing IIPToken for downstream consumers.Modules like SalesShareDistributor rely on
owner()/burnFrom()(currently not in IIPToken). If you plan to generalize those modules to any IIPToken (incl. WrappedIPToken), add the minimal read/actions here or keep those modules typed to concrete IPToken.src/SalesShareDistributor.sol (2)
137-142: Docstring type mismatch: IIPToken vs IPToken.The parameter doc says IIPToken, but the function takes IPToken. Align docs or generalize the function.
- * @param tokenContract IIPToken the IPToken token contract + * @param tokenContract IPToken the IPToken token contract
58-68: Rounding/thresholds: hardcoded 1000 minimums.Fixed thresholds can break for tokens with different decimals. Consider a percentage-based or 1 wei minimum.
script/prod/RolloutTokenizerV14.s.sol (1)
19-27: Good: emits deterministic reinit calldata for multisig.Add a checksum of the calldata to simplify out-of-band verification.
console.log("UpgradeCallData:"); console.logBytes(upgradeCallData); + console.log("UpgradeCallData keccak256:"); + console.logBytes32(keccak256(upgradeCallData));README.md (1)
103-109: Clarify upgrade bytes and provide an alternative using cast abi-encode.Reduce copy/paste errors by showing how to derive the bytes on demand.
-// 0xTokenizer (address, bytes)(0xNewImpl, 0xNewWrappedIPTokenImpl 0xNewIPTokenImpl) -cast send --rpc-url=$RPC_URL --private-key=$PRIVATE_KEY 0x58EB89C69CB389DBef0c130C6296ee271b82f436 "upgradeToAndCall(address,bytes)" 0x34A1D3fff3958843C43aD80F30b94c510645C316 0x8b3d19bb0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000005b73c5498c1e3b4dba84de0f1833c4a029d90519 +// 0xTokenizer (address, bytes)(0xNewImpl, abi.encodeWithSelector(reinit(address,address), 0xNewWrappedIPTokenImpl, 0xNewIPTokenImpl)) +cast send --rpc-url=$RPC_URL --private-key=$PRIVATE_KEY 0x58EB89C69CB389DBef0c130C6296ee271b82f436 \ + "upgradeToAndCall(address,bytes)" \ + 0x34A1D3fff3958843C43aD80F30b94c510645C316 \ + $(cast abi-encode "reinit(address,address)" 0xNewWrappedIPTokenImpl 0xNewIPTokenImpl)test/Tokenizer.t.sol (2)
42-46: Test double reaches into IPToken internals.Directly setting _metadata couples the test to internal layout. Prefer a minimal initializer/constructor hook on the fake or a public setter guarded by test-only code to reduce brittleness.
contract FakeIPT is IPToken { - constructor(uint256 ipnftId) { - _metadata = TokenMetadata({ ipnftId: ipnftId, originalOwner: msg.sender, agreementCid: "ipfs://agreementCid" }); - } + constructor(uint256 ipnftId) { + _setTestMetadata(ipnftId, msg.sender, "ipfs://agreementCid"); + } } + +// In IPToken (test-only build), add: +function _setTestMetadata(uint256 ipnftId, address originalOwner, string memory agreementCid) external { + _metadata = TokenMetadata({ ipnftId: ipnftId, originalOwner: originalOwner, agreementCid: agreementCid }); +}
117-123: Strengthen URI assertions.Asserting only length > 200 is weak. Also assert presence of key fields (ipnft_id, agreement_content).
- string memory uri = tokenContract.uri(); - assertGt(bytes(uri).length, 200); + string memory uri = tokenContract.uri(); + assertGt(bytes(uri).length, 200); + assertTrue(bytes(uri).length != 0); + assertTrue(bytes(string.concat(uri)).length > 0); // force memory read + assertTrue( + bytes(uri).length > 0 && + bytes(uri).find(bytes('"ipnft_id"')) != type(uint).max && + bytes(uri).find(bytes('"agreement_content"')) != type(uint).max + );test/TokenizerWrapped.t.sol (4)
19-36: Invalid-decimals stub is fine; optional: add balanceOf to future-proof.Tokenizer only reads metadata/decimals here, but adding a trivial balanceOf guards future changes.
contract FakeERC20WithInvalidDecimals { + function balanceOf(address) external pure returns (uint256) { + return 0; + }
85-97: Add assertions on URI and decimals forwarding.Also confirm the data-URI contains original ERC20 address and decimals value.
IIPToken tokenContract = tokenizer.attachIpt(1, agreementCid, "", erc20); @@ assertEq(tokenContract.name(), "URORiif"); + // Validate data-URI contains original ERC20 + decimals + string memory uri = WrappedIPToken(address(tokenContract)).uri(); + assertTrue(contains(uri, Strings.toHexString(address(erc20)))); + assertTrue(contains(uri, Strings.toString(erc20.decimals())));
136-149: Revert-message coupling.Tests assert revert strings; if we later switch to custom errors, these will break. Consider matching by selector/custom error instead, or keep as-is intentionally.
- vm.expectRevert("WrappedIPToken: cannot issue"); + // vm.expectRevert(WrappedIPToken.CannotIssue.selector); // if migrated to custom error
151-177: contains() is unused—use it or drop it.Either leverage it for URI checks (see above) or remove to avoid dead code in tests.
src/helpers/test-upgrades/IPToken13.sol (1)
11-16: Avoid duplicating Metadata; reuse the shared type.Keeps ABI/types aligned with IIPToken and reduces drift.
-import { IControlIPTs } from "../../IControlIPTs.sol"; - -struct Metadata { - uint256 ipnftId; - address originalOwner; - string agreementCid; -} +import { IControlIPTs } from "../../IControlIPTs.sol"; +import { IIPToken, Metadata } from "../../IIPToken.sol";src/WrappedIPToken.sol (2)
28-31: Add basic argument validation in initialize (defense-in-depth).Tokenizer validates upstream, but cheap checks here improve safety for direct uses.
function initialize(uint256 ipnftId, address originalOwner, string memory agreementCid, IERC20Metadata wrappedToken_) external initializer { - _metadata = Metadata({ ipnftId: ipnftId, originalOwner: originalOwner, agreementCid: agreementCid }); - wrappedToken = wrappedToken_; + require(address(wrappedToken_) != address(0), "WrappedIPToken: zero address"); + require(address(wrappedToken_).code.length > 0, "WrappedIPToken: not a contract"); + _metadata = Metadata({ ipnftId: ipnftId, originalOwner: originalOwner, agreementCid: agreementCid }); + wrappedToken = wrappedToken_; }
66-72: Consider custom errors for gas (optional).String reverts are fine for UX but cost more gas. If you decide to switch, update tests to expect errors by selector.
src/Permissioner.sol (1)
58-61: Signature scheme works; consider EIP‑712 in future.EIP-191 personal_sign is acceptable; EIP-712 would be a nicer UX and non-malleable structured data.
Would you like a follow-up PR sketching an EIP‑712 domain/separator and typed data for TermsAccepted?
src/Tokenizer.sol (4)
83-94: onlyController remains IPToken-typed by design — clarify intent.This intentionally excludes wrapped IPTs from Tokenizer.issue/cap. Consider a brief NatSpec note on issue/cap that they are legacy‑IPT only to avoid misuse/confusion.
116-126: Harden implementation setter: check code at the address.Guard against accidentally setting an EOA or uninitialized address.
+import { Address } from "@openzeppelin/contracts/utils/Address.sol"; @@ - if (address(_wrappedIpTokenImplementation) == address(0)) { + if (address(_wrappedIpTokenImplementation) == address(0)) { revert ZeroAddress(); - } + } + if (!Address.isContract(address(_wrappedIpTokenImplementation))) { + revert InvalidTokenContract(); + }If you prefer distinct errors, introduce an ImplementationNotContract() error instead of reusing InvalidTokenContract.
182-221: attachIpt: metadata calls can revert on non‑standard ERC‑20s; add try/catch fallback.You already validate decimals via IERC20Metadata. Some ERC‑20s still revert on name()/symbol(). Avoid hard reverts and still allow wrapping.
- emit TokensCreated( + // fetch token metadata defensively + string memory _name; + string memory _symbol; + try tokenContract.name() returns (string memory n) { _name = n; } catch { _name = ""; } + try tokenContract.symbol() returns (string memory s) { _symbol = s; } catch { _symbol = ""; } + uint256 _supply = tokenContract.totalSupply(); // standard; ok to bubble up if non-compliant + + emit TokensCreated( uint256(keccak256(abi.encodePacked(ipnftId))), ipnftId, address(tokenContract), _msgSender(), - tokenContract.totalSupply(), + _supply, agreementCid, - tokenContract.name(), - tokenContract.symbol() + _name, + _symbol );Optional: emit a TokenMetadataMissing event when name/symbol are empty to aid indexers.
251-274: Replace extcodesize with Address.isContract for readability.Semantics are the same; the library method is clearer and avoids inline assembly.
- uint256 codeSize; - assembly { - codeSize := extcodesize(tokenContract) - } - if (codeSize == 0) { - revert InvalidTokenContract(); - } + if (!Address.isContract(address(tokenContract))) { + revert InvalidTokenContract(); + }Also consider distinguishing “missing decimals()” from “too large” via separate errors for better UX/analytics.
test/Forking/Tokenizer14UpgradeForkTest.t.sol (4)
5-5: Remove unused import.console is not used.
-import { console } from 'forge-std/console.sol';
23-24: Drop unused SafeERC20 using.Not used with IPToken in this test file.
- using SafeERC20Upgradeable for IPToken;
83-114: Avoid brittle revert string matching for Initializable.Prefer expectRevert() without a string for OZ initializer guards to reduce coupling to message text.
- vm.expectRevert('Initializable: contract is already initialized'); + vm.expectRevert(); tokenizer14.initialize(IPNFT(address(0)), BlindPermissioner(address(0)));(Repeat similarly for other initializer assertions.)
36-36: Gitleaks “generic API key” is likely a false positive (Ethereum address), but verify.Those hex strings are EOAs/contracts. If Gitleaks persists, add a repo allowlist rule for Eth addresses in test files to suppress noise.
I can draft a minimal .gitleaks.toml allowlist entry scoped to test/Forking/*.t.sol to ignore EVM addresses while still catching real secrets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
.github/workflows/test.yml(1 hunks)README.md(1 hunks)script/dev/Synthesizer.s.sol(0 hunks)script/dev/Tokenizer.s.sol(3 hunks)script/prod/RolloutTokenizerV13.s.sol(0 hunks)script/prod/RolloutTokenizerV14.s.sol(1 hunks)src/IIPToken.sol(1 hunks)src/IPToken.sol(5 hunks)src/Permissioner.sol(5 hunks)src/SalesShareDistributor.sol(2 hunks)src/Tokenizer.sol(7 hunks)src/WrappedIPToken.sol(1 hunks)src/helpers/test-upgrades/IPToken13.sol(1 hunks)src/helpers/test-upgrades/Tokenizer12.sol(0 hunks)src/helpers/test-upgrades/Tokenizer13.sol(1 hunks)test/CrowdSalePermissioned.t.sol(1 hunks)test/Forking/Tokenizer13UpgradeForkTest.t.sol(0 hunks)test/Forking/Tokenizer14UpgradeForkTest.t.sol(1 hunks)test/Mintpass.t.sol(0 hunks)test/Permissioner.t.sol(1 hunks)test/Tokenizer.t.sol(3 hunks)test/TokenizerWrapped.t.sol(1 hunks)test/helpers/Strings.sol(1 hunks)
💤 Files with no reviewable changes (5)
- script/prod/RolloutTokenizerV13.s.sol
- test/Mintpass.t.sol
- script/dev/Synthesizer.s.sol
- test/Forking/Tokenizer13UpgradeForkTest.t.sol
- src/helpers/test-upgrades/Tokenizer12.sol
🧰 Additional context used
🪛 Gitleaks (8.27.2)
test/Forking/Tokenizer14UpgradeForkTest.t.sol
[high] 36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (26)
test/helpers/Strings.sol (1)
7-7: No-op change looks fine.Allocation expression is equivalent; no semantic difference.
src/IIPToken.sol (1)
16-16: Clarify cap() semantics; consider explicit naming or return type.As an interface,
function cap() external;is ambiguous (action vs getter). If it finalizes issuance, considerfinalizeCap()/seal(); if it’s a getter, make itview returns (uint256).Would you confirm intended behavior and update the signature/name accordingly?
test/CrowdSalePermissioned.t.sol (1)
11-12: LGTM: import path aligns with Metadata move.Change is consistent with centralizing Metadata in IIPToken.sol.
test/Permissioner.t.sol (1)
6-8: LGTM: imports updated to the new Metadata location.No behavioral impact.
script/dev/Tokenizer.s.sol (2)
8-11: LGTM: imports reflect IIPToken/Wrapper split.Consistent with the BYOT wrapper integration.
29-36: LGTM: wiring WrappedIPToken into Tokenizer.Setting both implementations post-initialize is clear; logs help debugging.
Optionally assert on-chain that
tokenizer.wrappedIPTokenImplementation()equals the deployed address after this step.test/Tokenizer.t.sol (5)
20-21: Import split looks correct.Switching Metadata to come from IIPToken and keeping IPToken import for concrete type usage is aligned with the new interface split.
26-28: controllerOf made pure is fine for the test harness.Returning address(0) simplifies gating; owner-based checks still pass after handover since the governor becomes owner.
Please confirm no tests rely on a non-zero controller when the governor is owner (shouldn’t).
218-228: Good negative test for fake tokens.Covers IPTNotControlledByTokenizer as intended.
265-296: Consider adding a negative case for unauthorized handover.Add a check that a non-controller cannot call handoverControl, expecting MustControlIpnft.
htokenizer.handoverControl(tokenContract, governor); + // Non-controller cannot hand over + vm.startPrank(bob); + vm.expectRevert(MustControlIpnft.selector); + htokenizer.handoverControl(tokenContract, governor);
299-318: Ownership-transfer semantics test is solid.Confirms previous owner cannot issue post-transfer while new owner can.
test/TokenizerWrapped.t.sol (3)
62-76: Setup wires both implementations correctly.Ensures Tokenizer can mint native IPTs and wrap ERC20s.
98-109: Great negative coverage; add non-owner and double-attach cases.
- Non-IPNFT-owner attach should revert MustControlIpnft.
- Attaching twice (or after tokenizeIpnft) should revert AlreadyTokenized.
vm.expectRevert(InvalidTokenContract.selector); tokenizer.attachIpt(1, agreementCid, "", IERC20Metadata(alice)); + + // Non-owner cannot attach + vm.stopPrank(); + vm.startPrank(alice); + vm.expectRevert(MustControlIpnft.selector); + tokenizer.attachIpt(1, agreementCid, "", IERC20Metadata(address(erc20))); + + // Owner attaches once, second attach reverts + vm.stopPrank(); + vm.startPrank(originalOwner); + tokenizer.attachIpt(1, agreementCid, "", erc20); + vm.expectRevert(AlreadyTokenized.selector); + tokenizer.attachIpt(1, agreementCid, "", erc20);
120-134: Casts and property checks look good.Casts to WrappedIPToken and property assertions are correct.
src/helpers/test-upgrades/IPToken13.sol (1)
87-119: URI JSON is consistent with 1.3; OK.Emits 18 decimals and expected properties; matches existing expectations.
src/WrappedIPToken.sol (1)
74-108: URI includes original ERC20 and decimals—good.Matches BYO-token requirements and aids indexers.
src/Permissioner.sol (2)
7-20: Interface migration to IIPToken is correct.Public API now consumes IIPToken+Metadata; no behavior change.
81-83: Wrapper overload is clean.Delegates to the Metadata version; OK.
src/Tokenizer.sol (3)
39-42: Events shape looks right; wrapper emits original ERC20 in TokensCreated.Good call emitting the underlying token address for subgraphs and adding TokenWrapped/WrappedIPTokenImplementationUpdated.
131-134: Reinitializer(6) usage is appropriate.Chaining to both setters is clean and keeps event emissions. No issues.
47-47: Storage-layout verified — IPToken → IIPToken is storage-compatible; wrappedTokenImplementation appended.Mapping value change is address-compatible; synthesized remains at the same position (see src/helpers/test-upgrades/Tokenizer13.sol vs src/Tokenizer.sol) and wrappedTokenImplementation is declared after ipTokenImplementation, so no slot reordering detected — upgrade OK.
src/IPToken.sol (3)
23-41: IIPToken integration and initializer look good.Implements interface cleanly; owner remains the Tokenizer via initializer context. No blockers.
58-72: Overrides are explicit and correct.Multiple‑inheritance overrides resolve to ERC20Upgradeable; good.
116-124: URI external_url change acknowledged.If docs or subgraphs depend on molecule.to, ensure they’re updated.
src/helpers/test-upgrades/Tokenizer13.sol (2)
115-123: Hardcoded legacy synthesized mapping — confirm mainnet snapshot.These addresses must match the fork block used in tests. If block/tag changes, tests will fail.
Add a short comment indicating the expected block number (23367545) to keep future maintainers aligned.
134-172: Legacy tokenize flow aligns with v14; no issues.Clone, initialize, permissioner.accept, then issue — consistent with new flow.
0xmme
force-pushed
the
feat/bring-your-own-ipt
branch
from
f3ba328 to
7d10bf0
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 (2)
src/crowdsale/LockingCrowdSale.sol (1)
49-51: Enforce TimelockedToken's underlying token matches the provided token.TimelockedToken exposes a public underlyingToken; compare addresses to avoid interface-type mismatches. Update src/crowdsale/LockingCrowdSale.sol (trustLockingContract):
function trustLockingContract(IERC20 token, TimelockedToken _timelockedToken) public onlyOwner { require(address(_timelockedToken.underlyingToken()) == address(token), "Mismatched underlying"); lockingContracts[address(token)] = _timelockedToken; }src/helpers/test-upgrades/Tokenizer13.sol (1)
55-60: Add zero-address guards in initialize.Prevent accidental misconfigurations.
function initialize(IPNFT _ipnft, IPermissioner _permissioner) external initializer { __UUPSUpgradeable_init(); __Ownable_init(); - ipnft = _ipnft; - permissioner = _permissioner; + if (address(_ipnft) == address(0) || address(_permissioner) == address(0)) revert ZeroAddress(); + ipnft = _ipnft; + permissioner = _permissioner; }
🧹 Nitpick comments (21)
script/DeployShareDistributor.s.sol (1)
14-20: Minor hardening and hygiene.
- Validate
SOS_ADDRESSis set and non‑zero; optionally assertsosAddress.code.length > 0.- Consider
vm.labelon impl/proxy for better traces.- Remove unused imports (
IPNFT,Tokenizer) to quiet solc warnings.Suggested snippet (outside changed hunk):
require(sosAddress != address(0), "SOS_ADDRESS not set"); require(sosAddress.code.length > 0, "SOS_ADDRESS has no code"); vm.label(address(impl), "SalesShareDistributor_Impl"); vm.label(address(salesShareDistributor), "SalesShareDistributor_Proxy");test/ContractReceivers.t.sol (2)
80-82: Initialize the proxy in-constructor to avoid a transiently uninitialized proxy.Prefer encoding the initializer into the ERC1967Proxy constructor; it reduces one external call and mirrors prod-grade deployment patterns.
Apply:
- ipnft = IPNFT(address(new ERC1967Proxy(address(new IPNFT()), ""))); - ipnft.initialize(); + ipnft = IPNFT(address(new ERC1967Proxy(address(new IPNFT()), abi.encodeCall(IPNFT.initialize, ())))); + // ipnft.initialize(); // already executed via proxy constructorAlt (if you keep separate initialize): use explicit bytes to avoid any linting around literal types.
- ipnft = IPNFT(address(new ERC1967Proxy(address(new IPNFT()), ""))); + ipnft = IPNFT(address(new ERC1967Proxy(address(new IPNFT()), bytes(""))));
86-89: Quote normalization LGTM; consider Foundry’s ERC20 deal() for brevity.String-literal fix is fine. Minor: you can replace minting with vm.deal for ERC20 to shorten setup if FakeERC20 isn’t under test here.
Example:
// vm.deal for ERC20 vm.startPrank(deployer); deal(address(_testToken), buyer, 1 ether); vm.stopPrank();src/crowdsale/LockingCrowdSale.sol (4)
99-104: Allowance strategy: confirm additive approval is intentional.
forceApprove(spender, currentAllowance + sale.salesAmount)accumulates leftover allowance from previous sales to the same locking contract. That can leave a standing allowance > needed. If not required for overlapping sales, prefer setting to the exact expected total for this sale to reduce residual approval surface.If overlapping/parallel sales per token aren’t supported, switch to:
-uint256 currentAllowance = sale.auctionToken.allowance(address(this), address(lockingContract)); -sale.auctionToken.forceApprove(address(lockingContract), currentAllowance + sale.salesAmount); +sale.auctionToken.forceApprove(address(lockingContract), sale.salesAmount);
112-123: Edge-case UX: use >= so tokens transfer directly at exact unlock time.With
>, claims atblock.timestamp == closingTime + durationstill go throughlock(...)even though the cliff has elapsed, forcing an extra release step. Using>=avoids unnecessary locking precisely at the boundary.- if (block.timestamp > _sales[saleId].closingTime + duration) { + if (block.timestamp >= _sales[saleId].closingTime + duration) {
34-36: Defensive check: require non-zero implementation address.Adding an explicit non-zero check prevents accidentally deploying a clone that delegates to address(0).
constructor(TimelockedToken _timelockedTokenImplementation) { - TIMELOCKED_TOKEN_IMPLEMENTATION = address(_timelockedTokenImplementation); + address impl = address(_timelockedTokenImplementation); + require(impl != address(0), "zero impl"); + TIMELOCKED_TOKEN_IMPLEMENTATION = impl; }
76-86: No bug — parent computes identical saleId; optional refactor: move mapping write after super.startSaleCrowdSale.startSale computes saleId = uint256(keccak256(abi.encode(sale))) (same as LockingCrowdSale's precompute), so the mapping write targets the correct key; moving salesLockingDuration[saleId] after saleId = super.startSale(sale) is a harmless defensive change to decouple from the parent implementation.
function startSale(Sale calldata sale, uint256 lockingDuration) public virtual returns (uint256 saleId) { - saleId = uint256(keccak256(abi.encode(sale))); - if (lockingDuration > 366 days) { revert InvalidDuration(); } createOrReturnTimelockContract(sale.auctionToken); - salesLockingDuration[saleId] = lockingDuration; - saleId = super.startSale(sale); + saleId = super.startSale(sale); + salesLockingDuration[saleId] = lockingDuration; }README.md (1)
103-109: Clarify the 1.4 upgrade calldata generationTo avoid copy/paste errors, include a reproducible snippet to build the reinit calldata.
-// 0xTokenizer (address, bytes)(0xNewImpl, 0xNewWrappedIPTokenImpl 0xNewIPTokenImpl) -cast send --rpc-url=$RPC_URL --private-key=$PRIVATE_KEY 0x58EB89C69CB389DBef0c130C6296ee271b82f436 "upgradeToAndCall(address,bytes)" 0x34A1D3fff3958843C43aD80F30b94c510645C316 0x8b3d19bb0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000005b73c5498c1e3b4dba84de0f1833c4a029d90519 +// Build calldata for reinit(address wrappedImpl, address ipImpl) +REINIT_CALLDATA=$(cast abi-encode "reinit(address,address)" 0xNewWrappedIPTokenImpl 0xNewIPTokenImpl) +cast send --rpc-url=$RPC_URL --private-key=$PRIVATE_KEY 0x58EB89C69CB389DBef0c130C6296ee271b82f436 \ + "upgradeToAndCall(address,bytes)" 0xNewTokenizerImpl $REINIT_CALLDATAAlso document that attachIpt requires IERC20Metadata compliance and decimals <= 18.
src/Tokenizer.sol (3)
116-126: Harden implementation setter: ensure code at addressAdd a code-size check to avoid configuring EOA/empty contracts as implementations.
function setWrappedIPTokenImplementation(WrappedIPToken _wrappedIpTokenImplementation) public onlyOwner { /* could call some functions on old contract to make sure its tokenizer not another contract behind a proxy for safety */ if (address(_wrappedIpTokenImplementation) == address(0)) { revert ZeroAddress(); } + if (address(_wrappedIpTokenImplementation).code.length == 0) { + revert InvalidTokenContract(); + } emit WrappedIPTokenImplementationUpdated(wrappedTokenImplementation, _wrappedIpTokenImplementation); wrappedTokenImplementation = _wrappedIpTokenImplementation; }
190-221: Guardrails and safer metadata reads in attachIpt
- Add explicit check that wrappedTokenImplementation is set before cloning (clearer error vs. low-level revert).
- Consider try/catch around name(), symbol(), totalSupply() to avoid griefing by non‑compliant tokens during event emission.
function attachIpt(uint256 ipnftId, string memory agreementCid, bytes calldata signedAgreement, IERC20Metadata tokenContract) external returns (IIPToken) { if (_msgSender() != controllerOf(ipnftId)) { revert MustControlIpnft(); } if (address(synthesized[ipnftId]) != address(0)) { revert AlreadyTokenized(); } // Sanity checks for token properties _validateTokenContract(tokenContract); + if (address(wrappedTokenImplementation) == address(0)) { + revert ZeroAddress(); // implementation not configured + } + WrappedIPToken wrappedIpt = WrappedIPToken(Clones.clone(address(wrappedTokenImplementation))); wrappedIpt.initialize(ipnftId, _msgSender(), agreementCid, tokenContract); synthesized[ipnftId] = wrappedIpt; - emit TokensCreated( + // Best-effort metadata for event logging + uint256 supply = 0; + string memory name = ""; + string memory symbol = ""; + try tokenContract.totalSupply() returns (uint256 s) { supply = s; } catch {} + try tokenContract.name() returns (string memory n) { name = n; } catch {} + try tokenContract.symbol() returns (string memory s) { symbol = s; } catch {} + + emit TokensCreated( uint256(keccak256(abi.encodePacked(ipnftId))), ipnftId, address(tokenContract), _msgSender(), - tokenContract.totalSupply(), - agreementCid, - tokenContract.name(), - tokenContract.symbol() + supply, + agreementCid, + name, + symbol ); emit TokenWrapped(tokenContract, wrappedIpt); permissioner.accept(wrappedIpt, _msgSender(), signedAgreement); return wrappedIpt; }If strict IERC20Metadata is required, instead add explicit checks and revert with a dedicated error on empty name/symbol.
251-274: Prefer code.length over assembly and clarify decimals policy
- Replace extcodesize with address(tokenContract).code.length for readability.
- Document or relax the “<= 18” rule if tokens with >18 decimals must be supported.
- uint256 codeSize; - assembly { - codeSize := extcodesize(tokenContract) - } - if (codeSize == 0) { + if (address(tokenContract).code.length == 0) { revert InvalidTokenContract(); }Optionally change the error to include the observed decimals for easier debugging: InvalidTokenDecimals(uint8 actual).
test/Forking/Tokenizer14UpgradeForkTest.t.sol (8)
7-7: Remove unused SafeERC20Upgradeable import/using.The library isn’t used; drop the import and the
usingto keep the test lean.Apply:
-import { SafeERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol"; ... -contract Tokenizer14UpgradeForkTest is Test { - using SafeERC20Upgradeable for IPToken; +contract Tokenizer14UpgradeForkTest is Test {Also applies to: 24-24
33-39: Mark mainnet constants as immutable constants.These never change in the test; prefer
constantfor clarity and gas in setup.-address mainnetDeployer = 0x34021576F01275A429163a56908Bd02b43e2B7e1; -address mainnetOwner = 0xCfA0F84660fB33bFd07C369E5491Ab02C449f71B; -address mainnetTokenizer = 0x58EB89C69CB389DBef0c130C6296ee271b82f436; -address mainnetIPNFT = 0xcaD88677CA87a7815728C72D74B4ff4982d54Fc1; +address constant mainnetDeployer = 0x34021576F01275A429163a56908Bd02b43e2B7e1; +address constant mainnetOwner = 0xCfA0F84660fB33bFd07C369E5491Ab02C449f71B; +address constant mainnetTokenizer = 0x58EB89C69CB389DBef0c130C6296ee271b82f436; +address constant mainnetIPNFT = 0xcaD88677CA87a7815728C72D74B4ff4982d54Fc1;
76-95: Minor prank hygiene.Starting a new prank overrides the prior one, but add
vm.stopPrank()before switching signers for readability.
100-107: Permissioner hot-swap via deployCodeTo: call out test-only hack.Overwriting code at
permissioner()is intentional in forks but brittle. Add a comment that this is a test-only shortcut.
254-290: Add event assertion for TokensCreated on fresh tokenize.Validate the emitted event to guard regressions.
// Tokenize it with the new implementation - IPToken newToken = tokenizer14.tokenizeIpnft(newIpnftId, 1_000_000 ether, "NEW-IPT", agreementCid, bytes("")); + // Expect event (topics only: ipnftId and token address) + vm.expectEmit(false, true, true, false); + emit TokensCreated(0, newIpnftId, address(0), address(0), 0, "", "", ""); + IPToken newToken = tokenizer14.tokenizeIpnft(newIpnftId, 1_000_000 ether, "NEW-IPT", agreementCid, bytes(""));Add the event signature near the top of the test (once):
contract Tokenizer14UpgradeForkTest is Test { + event TokensCreated( + uint256 indexed moleculesId, + uint256 indexed ipnftId, + address indexed tokenContract, + address emitter, + uint256 amount, + string agreementCid, + string name, + string symbol + );
358-421: BYOT: assert TokensCreated references the original ERC20 (not the wrapper).This is a core PR objective; add an event check so subgraph-facing behavior is locked.
- // Attach the ERC20 token to the IPNFT using the new V14 functionality + // Expect the Tokenizer to reference the original token in TokensCreated + vm.expectEmit(false, true, true, false); + emit TokensCreated(0, newIpnftId, address(testToken), address(0), 0, "", "", ""); + // Attach the ERC20 token to the IPNFT using the new V14 functionality IIPToken wrappedToken = tokenizer14.attachIpt(newIpnftId, agreementCid, bytes(""), testToken);Also add a negative-path check (non-owner cannot attach):
+ // Non-owner cannot attach + address bob = makeAddr("bob"); + vm.startPrank(bob); + vm.expectRevert(MustControlIpnft.selector); + tokenizer14.attachIpt(newIpnftId, agreementCid, bytes(""), testToken); + vm.stopPrank();And prefer asserting specific revert selectors for wrapper ops if available instead of generic
expectRevert().
380-399: Consider a decimals≠18 case for wrapped tokens.Add a 6‑decimals ERC20 to ensure wrapper surfaces underlying decimals correctly.
I can add a tiny FakeERC20 with overridable decimals and extend this test.
304-309: Legacy name expectation: guard against string change.If legacy token names ever get normalized, this strict string check will flake. Consider checking prefix/suffix only.
src/helpers/test-upgrades/Tokenizer13.sol (2)
72-83: onlyController reads full Metadata struct; consider lighter check.Pulling the whole struct (incl. strings) is costlier than needed. If available, prefer a minimal
ipnftId()getter or a cheap mapping.Do you have an inexpensive way to fetch only
ipnftIdfrom IPToken?
121-156: Emit after permission is accepted (optional).If
permissioner.acceptreverts, the event would have been emitted prematurely. Consider accepting first, then emitting, unless subgraphs require current order.Confirm event ordering requirements for downstream consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (41)
.github/workflows/test.yml(1 hunks)README.md(1 hunks)script/DeployShareDistributor.s.sol(1 hunks)script/IPNFT.s.sol(1 hunks)script/dev/CrowdSale.s.sol(1 hunks)script/dev/Ipnft.s.sol(1 hunks)script/dev/Synthesizer.s.sol(0 hunks)script/dev/Tokenizer.s.sol(2 hunks)script/prod/RolloutTokenizerV13.s.sol(0 hunks)script/prod/RolloutTokenizerV14.s.sol(1 hunks)script/prod/RolloutV23.sol(1 hunks)script/prod/RolloutV25Sale.s.sol(3 hunks)src/BioPriceFeed.sol(0 hunks)src/IIPToken.sol(1 hunks)src/IPNFT.sol(1 hunks)src/IPToken.sol(5 hunks)src/Permissioner.sol(5 hunks)src/SalesShareDistributor.sol(2 hunks)src/Tokenizer.sol(7 hunks)src/WrappedIPToken.sol(1 hunks)src/crowdsale/LockingCrowdSale.sol(2 hunks)src/crowdsale/StakedLockingCrowdSale.sol(2 hunks)src/helpers/test-upgrades/IPToken13.sol(1 hunks)src/helpers/test-upgrades/Tokenizer13.sol(5 hunks)test/BioPriceFeed.t.sol(1 hunks)test/ContractReceivers.t.sol(1 hunks)test/CrowdSale.t.sol(1 hunks)test/CrowdSaleFuzz.t.sol(1 hunks)test/CrowdSaleLockedStakedTest.t.sol(5 hunks)test/CrowdSaleLockedTest.t.sol(1 hunks)test/CrowdSalePermissioned.t.sol(1 hunks)test/CrowdSaleWithNonStandardERC20Test.t.sol(2 hunks)test/Forking/Tokenizer13UpgradeForkTest.t.sol(0 hunks)test/Forking/Tokenizer14UpgradeForkTest.t.sol(1 hunks)test/IPNFT.t.sol(1 hunks)test/Mintpass.t.sol(0 hunks)test/Permissioner.t.sol(1 hunks)test/TimelockedToken.t.sol(1 hunks)test/Tokenizer.t.sol(3 hunks)test/TokenizerWrapped.t.sol(1 hunks)test/helpers/Strings.sol(1 hunks)
💤 Files with no reviewable changes (5)
- script/prod/RolloutTokenizerV13.s.sol
- test/Forking/Tokenizer13UpgradeForkTest.t.sol
- src/BioPriceFeed.sol
- test/Mintpass.t.sol
- script/dev/Synthesizer.s.sol
✅ Files skipped from review due to trivial changes (13)
- test/CrowdSaleFuzz.t.sol
- test/TimelockedToken.t.sol
- src/crowdsale/StakedLockingCrowdSale.sol
- test/IPNFT.t.sol
- test/CrowdSale.t.sol
- test/BioPriceFeed.t.sol
- script/prod/RolloutV23.sol
- script/dev/CrowdSale.s.sol
- script/dev/Ipnft.s.sol
- test/CrowdSaleLockedTest.t.sol
- script/IPNFT.s.sol
- src/IPNFT.sol
- script/prod/RolloutV25Sale.s.sol
🚧 Files skipped from review as they are similar to previous changes (13)
- src/IIPToken.sol
- .github/workflows/test.yml
- test/CrowdSalePermissioned.t.sol
- src/helpers/test-upgrades/IPToken13.sol
- src/WrappedIPToken.sol
- test/Tokenizer.t.sol
- test/Permissioner.t.sol
- script/prod/RolloutTokenizerV14.s.sol
- test/helpers/Strings.sol
- src/SalesShareDistributor.sol
- test/TokenizerWrapped.t.sol
- script/dev/Tokenizer.s.sol
- src/IPToken.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-09T14:07:29.973Z
Learnt from: elmariachi111
PR: moleculeprotocol/IPNFT#170
File: script/prod/RolloutV25Sale.s.sol:24-24
Timestamp: 2025-01-09T14:07:29.973Z
Learning: In the IPNFT project, TimelockedToken contract serves as a template for minimal clones and is intentionally initialized with address(0) as it has no real state. The actual token address is set when clones are created.
Applied to files:
src/crowdsale/LockingCrowdSale.solsrc/helpers/test-upgrades/Tokenizer13.sol
🪛 Gitleaks (8.27.2)
test/Forking/Tokenizer14UpgradeForkTest.t.sol
[high] 35-35: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (12)
src/crowdsale/LockingCrowdSale.sol (2)
29-29: LGTM: visibility/modifier order tweak is fine.Reordering to
public immutableis stylistic; no runtime or ABI impact beyond the generated getter name remaining the same.
44-48: Doc-only change acknowledged.Extra blank line in the NatSpec is fine; no impact.
test/CrowdSaleWithNonStandardERC20Test.t.sol (1)
43-46: Formatting-only changes — OKWhitespace and constructor call compaction only; no behavioral impact.
Also applies to: 58-59
test/CrowdSaleLockedStakedTest.t.sol (4)
49-53: Formatting-only change — OKConstructor arg spacing only.
61-62: TokenVesting constructor collapsed — OKSame arguments, no semantic change.
116-121: Updated invalid-duration path — OKSwitch to 3 days + 7 days as the valid boundary matches new constraints.
If the minimum vesting period is now 7 days across the board, confirm all other tests use 7-day boundaries consistently.
139-141: Minor test refactor — OKstartSale args and the explicit InvalidDuration revert look consistent with new validation.
Also applies to: 397-401
src/Tokenizer.sol (3)
6-12: Public surface changes impact indexers
- Events now reference IIPToken and add TokenWrapped/WrappedIPTokenImplementationUpdated.
- synthesized mapping switches to IIPToken.
Ensure subgraphs/clients updated for the new event ABIs and mapping return type.
Also applies to: 21-25, 39-42
47-48: Mapping type change: check external readsPublic getter now returns IIPToken (address). Verify any off-chain code that previously decoded as IPToken adjusts accordingly.
131-134: Upgrade runbook: call reinit v6 post-implementation upgradeReinit sets both implementations. Confirm rollout scripts always call reinit (v6) in the same tx as upgrade, as shown in README.
test/Forking/Tokenizer14UpgradeForkTest.t.sol (2)
51-53: Pin fork block and document drift risk.Block 23,367,545 is fine, but any state drift breaks hardcoded assertions. Add a note or env override to ease maintenance.
Would you like a tiny harness to allow overriding the block via
FOUNDRY_FORK_BLOCK?
35-35: Gitleaks false positive.The flagged “Generic API Key” is just an Ethereum address; no secret is exposed.
0xmme
force-pushed
the
feat/bring-your-own-ipt
branch
2 times, most recently
from
2505672 to
a88fd86
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 (3)
src/IPToken.sol (2)
54-56: metadata() should override IIPToken.Aligns with other overrides (name/symbol/decimals/balanceOf) and avoids interface conformance issues.
- function metadata() external view returns (Metadata memory) { + function metadata() external view override returns (Metadata memory) { return _metadata; }
99-131: uri() should override IIPToken.Keeps interface consistent with WrappedIPToken and IIPToken.
- function uri() external view returns (string memory) { + function uri() external view override returns (string memory) { string memory tokenId = Strings.toString(_metadata.ipnftId); @@ - '","description": "IP Tokens, derived from IP-NFTs, are ERC-20 tokens governing IP pools.","decimals": 18,"external_url": "https://molecule.xyz","image": "",', + '","description": "IP Tokens, derived from IP-NFTs, are ERC-20 tokens governing IP pools.","decimals": 18,"external_url": "https://molecule.xyz","image": "",',src/helpers/test-upgrades/Tokenizer13.sol (1)
56-61: Initialize: add zero-address validation for dependenciesCalling permissioner.accept later will hard-revert if permissioner is address(0).
Apply:
function initialize(IPNFT _ipnft, IPermissioner _permissioner) external initializer { __UUPSUpgradeable_init(); __Ownable_init(); - ipnft = _ipnft; - permissioner = _permissioner; + if (address(_ipnft) == address(0) || address(_permissioner) == address(0)) { + revert ZeroAddress(); + } + ipnft = _ipnft; + permissioner = _permissioner; }
♻️ Duplicate comments (1)
src/helpers/test-upgrades/Tokenizer13.sol (1)
8-9: Import source for Metadata fixed — good catchSwitching Metadata to IIPToken.sol de-couples from concrete implementation and matches the refactor.
🧹 Nitpick comments (16)
.github/workflows/test.yml (1)
28-33: CI: You dropped size reporting; either restore it or remove the TODO.If losing contract size telemetry was unintentional, keep both paths so CI doesn’t fail if --sizes isn’t supported.
- #todo: forge build --sizes + # Capture contract sizes when available (falls back to plain build) - name: Run Forge build run: | forge --version - forge build + forge build --sizes || forge build id: buildOptional: add a matrix gate or env check to skip mainnet‑fork tests when MAINNET_RPC_URL is unavailable to support PRs from forks.
src/WrappedIPToken.sol (2)
28-31: Initialize: add basic sanity checks for the wrapped token.Tokenizer validates, but local checks harden the template against misuse when cloned directly.
function initialize(uint256 ipnftId, address originalOwner, string memory agreementCid, IERC20Metadata wrappedToken_) external initializer { - _metadata = Metadata({ ipnftId: ipnftId, originalOwner: originalOwner, agreementCid: agreementCid }); - wrappedToken = wrappedToken_; + require(address(wrappedToken_) != address(0), "WrappedIPToken: zero token"); + uint256 codeSize; + assembly { codeSize := extcodesize(wrappedToken_) } + require(codeSize > 0, "WrappedIPToken: not a contract"); + // Also sanity-check decimals range used elsewhere (Tokenizer enforces <=18) + require(wrappedToken_.decimals() <= 18, "WrappedIPToken: invalid decimals"); + + _metadata = Metadata({ ipnftId: ipnftId, originalOwner: originalOwner, agreementCid: agreementCid }); + wrappedToken = wrappedToken_; }
66-72: Prefer custom errors over string reverts for issue/cap (gas, clarity).Optional tidy-up; keeps revert reasons consistent with Tokenizer.
+ error CannotIssue(); + error CannotCap(); @@ - function issue(address, uint256) public virtual override { - revert("WrappedIPToken: cannot issue"); - } + function issue(address, uint256) public virtual override { revert CannotIssue(); } @@ - function cap() public virtual override { - revert("WrappedIPToken: cannot cap"); - } + function cap() public virtual override { revert CannotCap(); }src/IPToken.sol (1)
34-41: Use calldata for agreementCid to save gas.Matches the other string params and avoids an extra copy.
- function initialize(uint256 ipnftId, string calldata name_, string calldata symbol_, address originalOwner, string memory agreementCid) + function initialize(uint256 ipnftId, string calldata name_, string calldata symbol_, address originalOwner, string calldata agreementCid) external initializer { __Ownable_init(); __ERC20_init(name_, symbol_); _metadata = Metadata({ ipnftId: ipnftId, originalOwner: originalOwner, agreementCid: agreementCid }); }src/Tokenizer.sol (3)
39-41: Index TokenWrapped event params for easy subgraphing.Indexing both sides aids lookups by either address.
- event TokenWrapped(IERC20Metadata tokenContract, IIPToken wrappedIpt); + event TokenWrapped(IERC20Metadata indexed tokenContract, IIPToken indexed wrappedIpt);
247-274: _validateTokenContract: optionally sanity‑check name/symbol length.Edge‑case UX: extremely long or empty names/symbols can make URIs unwieldy. Consider lightweight checks (non‑empty, reasonable length).
201-221: attachIpt: consider emitting a distinct “attached” Amount.You emit TokensCreated.amount as totalSupply of the external ERC‑20. That’s fine, but downstream consumers might misread as “minted by Tokenizer.” Consider a dedicated field or document this in the subgraph/schema.
test/Forking/Tokenizer14UpgradeForkTest.t.sol (5)
5-5: Remove unused console import.-import { console } from "forge-std/console.sol";
24-24: Remove unused SafeERC20Upgradeable usage.Not used with IPToken in this test.
- using SafeERC20Upgradeable for IPToken;
76-95: End pranks explicitly to avoid leakage between expectations.Not strictly required (later startPrank overrides), but explicit stop improves test hygiene.
vm.expectRevert("Initializable: contract is already initialized"); newIPTokenImplementation.initialize(2, "Foo", "Bar", alice, "abcde"); - vm.startPrank(mainnetOwner); + vm.stopPrank(); + vm.startPrank(mainnetOwner); @@ - tokenizer14.reinit(newWrappedIPTokenImplementation, newIPTokenImplementation); + tokenizer14.reinit(newWrappedIPTokenImplementation, newIPTokenImplementation); + vm.stopPrank();
358-419: Tighten revert assertions for WrappedIPToken.issue/cap.Match the specific revert reasons to avoid false positives if another revert occurs.
- vm.expectRevert(); // WrappedIPToken should not allow issue/cap operations + vm.expectRevert(bytes4(keccak256("CannotIssue()"))); // or the exact string if keeping string errors WrappedIPToken(address(wrappedToken)).issue(alice, 1000 ether); - vm.expectRevert(); // WrappedIPToken should not allow cap operations + vm.expectRevert(bytes4(keccak256("CannotCap()"))); // update if using custom errors WrappedIPToken(address(wrappedToken)).cap();If you keep string reverts, use:
- vm.expectRevert(); + vm.expectRevert(bytes("WrappedIPToken: cannot issue"));and similarly for cap.
51-53: Fork height is pinned; consider env‑overridable height for resiliency.Mainnet state drift can break assertions. Make the block number configurable with a default.
- mainnetFork = vm.createFork(vm.envString("MAINNET_RPC_URL"), 23367545); + uint256 forkBlock = vm.envOr("FORK_BLOCK", uint256(23367545)); + mainnetFork = vm.createFork(vm.envString("MAINNET_RPC_URL"), forkBlock);src/helpers/test-upgrades/Tokenizer13.sol (4)
90-100: Harden implementation setter with code-existence checkCurrently only zero-address is checked. Also assert the target has code to avoid bricking clones.
Apply:
+error NotAContract(); @@ - function setIPTokenImplementation(IPToken _ipTokenImplementation) public onlyOwner { + function setIPTokenImplementation(IPToken _ipTokenImplementation) public onlyOwner { @@ if (address(_ipTokenImplementation) == address(0)) { revert ZeroAddress(); } + if (address(_ipTokenImplementation).code.length == 0) { + revert NotAContract(); + }
137-142: Guard against unset ipTokenImplementation before cloningCloning a zero implementation will revert unpredictably.
Apply:
@@ - token = IPToken(Clones.clone(address(ipTokenImplementation))); + if (address(ipTokenImplementation) == address(0)) revert ZeroAddress(); + token = IPToken(Clones.clone(address(ipTokenImplementation)));
145-154: Emit TokensCreated after successful accept() and issue()Events before side-effects are fine if later revert, but emitting after success better reflects final state in logs/subgraphs.
Apply:
- emit TokensCreated( - uint256(keccak256(abi.encodePacked(ipnftId))), - ipnftId, - address(token), - _msgSender(), - tokenAmount, - agreementCid, - name, - tokenSymbol - ); - permissioner.accept(token, _msgSender(), signedAgreement); - token.issue(_msgSender(), tokenAmount); + permissioner.accept(token, _msgSender(), signedAgreement); + token.issue(_msgSender(), tokenAmount); + emit TokensCreated( + uint256(keccak256(abi.encodePacked(ipnftId))), + ipnftId, + address(token), + _msgSender(), + tokenAmount, + agreementCid, + name, + tokenSymbol + );
35-36: Event PermissionerUpdated is declared but no setter uses itEither wire a setter or drop the event to avoid dead code in the helper.
If you want a setter:
+ function setPermissioner(IPermissioner _new) external onlyOwner { + if (address(_new) == address(0)) revert ZeroAddress(); + emit PermissionerUpdated(permissioner, _new); + permissioner = _new; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
.github/workflows/test.yml(1 hunks)README.md(1 hunks)script/dev/Synthesizer.s.sol(0 hunks)script/dev/Tokenizer.s.sol(2 hunks)script/prod/RolloutTokenizerV13.s.sol(0 hunks)script/prod/RolloutTokenizerV14.s.sol(1 hunks)src/IIPToken.sol(1 hunks)src/IPToken.sol(5 hunks)src/Permissioner.sol(5 hunks)src/Tokenizer.sol(7 hunks)src/WrappedIPToken.sol(1 hunks)src/helpers/test-upgrades/IPToken13.sol(1 hunks)src/helpers/test-upgrades/Tokenizer13.sol(5 hunks)test/CrowdSalePermissioned.t.sol(1 hunks)test/Forking/Tokenizer13UpgradeForkTest.t.sol(0 hunks)test/Forking/Tokenizer14UpgradeForkTest.t.sol(1 hunks)test/Mintpass.t.sol(0 hunks)test/Permissioner.t.sol(1 hunks)test/Tokenizer.t.sol(3 hunks)test/TokenizerWrapped.t.sol(1 hunks)
💤 Files with no reviewable changes (4)
- script/prod/RolloutTokenizerV13.s.sol
- script/dev/Synthesizer.s.sol
- test/Mintpass.t.sol
- test/Forking/Tokenizer13UpgradeForkTest.t.sol
🚧 Files skipped from review as they are similar to previous changes (10)
- test/TokenizerWrapped.t.sol
- script/prod/RolloutTokenizerV14.s.sol
- test/Permissioner.t.sol
- src/IIPToken.sol
- test/Tokenizer.t.sol
- src/Permissioner.sol
- src/helpers/test-upgrades/IPToken13.sol
- script/dev/Tokenizer.s.sol
- README.md
- test/CrowdSalePermissioned.t.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-09T14:07:29.973Z
Learnt from: elmariachi111
PR: moleculeprotocol/IPNFT#170
File: script/prod/RolloutV25Sale.s.sol:24-24
Timestamp: 2025-01-09T14:07:29.973Z
Learning: In the IPNFT project, TimelockedToken contract serves as a template for minimal clones and is intentionally initialized with address(0) as it has no real state. The actual token address is set when clones are created.
Applied to files:
src/helpers/test-upgrades/Tokenizer13.sol
🔇 Additional comments (10)
src/Tokenizer.sol (1)
83-95: Confirm intent: onlyController accepts IPToken, not IIPToken.This prevents using Tokenizer.issue/cap with WrappedIPToken (which is fine since wrapped IPTs can’t mint/cap). Just confirming this exclusion is deliberate.
src/helpers/test-upgrades/Tokenizer13.sol (9)
12-12: Adopting IControlIPTs is correct for controller-gated opsInterface alignment looks good.
14-17: Purpose-built custom errors — LGTMNames/readability are clear and used appropriately.
69-71: Helper getter is finePublic view exposing IPNFT contract aids tests and scripts.
73-85: onlyController guard is sound but depends on metadata() availabilityChecks both controller and that the ipToken is the synthesized one; safe for reentrancy. No changes requested.
Please confirm every ipToken used here implements metadata() exactly as IIPToken.Metadata to avoid ABI mismatch.
129-134: Controller and duplicate-tokenization checks — LGTMAccess control and AlreadyTokenized guard are correct.
165-167: issue passthrough with onlyController — LGTMMatches interface and preserves controller gating.
174-176: cap passthrough with onlyController — LGTMNo concerns.
178-181: controllerOf delegated to IPNFT.ownerOf — LGTMConsistent with current controller model; future governance can override here.
105-111: Hardcoded legacy IPT addresses — validate network assumptionsThese addresses are hardcoded in the upgrade helper and are also referenced by forking tests; confirm the test environment/CI pins the intended mainnet fork or make the addresses configurable.
Locations: src/helpers/test-upgrades/Tokenizer13.sol:106–108; test/Forking/Tokenizer14UpgradeForkTest.t.sol:100–102, 124, 144, 174, 211, 298.
Actions (pick one):
- Pin the fork/RPC + block in tests/CI so these mainnet IPT addresses are valid, or
- Move the addresses to a single named test constant / env var with a comment stating these are mainnet legacy IPTs and guard tests accordingly.
0xmme
force-pushed
the
feat/bring-your-own-ipt
branch
from
a88fd86 to
d2ebf2f
Compare
- drops stoneage Synthesizer - extracts IIPToken interface to be reused on Tokenizer and Permissioner - fixes linearization of dependencies on IPToken - adds general erc20 metadata fields to shared IPT interface - wrapping test case - rollout script and disable fork test - drops size checks on builds - add sanity checks for ipt attachment on tokenizer
0xmme
force-pushed
the
feat/bring-your-own-ipt
branch
from
d2ebf2f to
df3dcf3
Compare
nour-karoui
force-pushed
the
feat/bring-your-own-ipt
branch
from
ba431b8 to
14fd019
Compare
…h an existing ERC20 as an IPT
nour-karoui
force-pushed
the
feat/bring-your-own-ipt
branch
from
14fd019 to
af9f607
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
script/dev/Ipnft.s.sol (1)
88-90: Remove or use the second minted IPNFT.
token2Idis minted but unused. Either use it (e.g., second listing) or drop the mint to avoid confusion.- uint256 token2Id = mintIpnft(bob, bob); + // uint256 token2Id = mintIpnft(bob, bob); // Uncomment when neededscript/dev/Tokenizer.s.sol (1)
75-78: Fix typo in comment.- // Mmint a new IPT + // Mint a new IPTtest/TokenizerWrapped.t.sol (2)
85-97: Assert synthesized mapping points to the wrapper.Strengthen the test by asserting the synthesized entry equals the returned wrapper.
IIPToken tokenContract = tokenizer.attachIpt(1, agreementCid, "", erc20); assertEq(tokenContract.balanceOf(originalOwner), 1_000_000 ether); - assertNotEq(address(tokenizer.synthesized(1)), address(erc20)); // the synthesized member tracks the wrapped ipt + // the synthesized member tracks the wrapped ipt (not the underlying ERC20) + assertNotEq(address(tokenizer.synthesized(1)), address(erc20)); + assertEq(address(tokenizer.synthesized(1)), address(tokenContract)); assertEq(tokenContract.totalIssued(), 1_000_000 ether); assertEq(tokenContract.name(), "URORiif");
151-177: Remove unusedcontainshelper.Not referenced; can be dropped to keep the test lean.
- // Helper function to check if a string contains a substring - function contains(string memory source, string memory search) internal pure returns (bool) { - bytes memory sourceBytes = bytes(source); - bytes memory searchBytes = bytes(search); - - if (searchBytes.length > sourceBytes.length) { - return false; - } - - for (uint256 i = 0; i <= sourceBytes.length - searchBytes.length; i++) { - bool found = true; - - for (uint256 j = 0; j < searchBytes.length; j++) { - if (sourceBytes[i + j] != searchBytes[j]) { - found = false; - break; - } - } - - if (found) { - return true; - } - } - - return false; - }src/Tokenizer.sol (3)
59-61: Guard against unset wrapped implementation.If
wrappedTokenImplementationis unset,Clones.clonewill produce a broken instance. Add an explicit zero-address guard inattachIpt.function attachIpt(uint256 ipnftId, string memory agreementCid, bytes calldata signedAgreement, IERC20Metadata tokenContract) external returns (IIPToken) { + if (address(wrappedTokenImplementation) == address(0)) { + revert ZeroAddress(); + }
182-190: Fix NatSpec return type.Comment says
@return IPTokenbut function returnsIIPToken.- * @return IPToken a wrapped IPToken that represents the tokenized ipnft for permissioners and carries metadata + * @return IIPToken a wrapped IPToken that represents the tokenized ipnft for permissioners and carries metadata
190-221: attachIpt flow and events LGTM; add the zero-address guard.Validation, clone, metadata init, events, and permissioner acceptance are all in order. Include the zero-address check above to harden against misconfiguration.
src/Permissioner.sol (2)
15-19: Fix NatSpec param types: these still say IPToken but the code uses IIPToken.Update the docs to avoid confusion when consuming the interface.
Apply:
- * @param tokenContract IPToken + * @param tokenContract IIPTokenAlso applies to: 43-47, 56-58, 79-83
58-61: Optional: bind the signature message to the token contract address.You already include ipnftId and chainid. Including the token contract address eliminates ambiguity in edge cases (e.g., future re-wraps or mirrors on the same chain).
For example:
- bytes32 termsHash = ECDSA.toEthSignedMessageHash(bytes(specificTermsV1(tokenContract))); + bytes32 termsHash = ECDSA.toEthSignedMessageHash( + abi.encodePacked( + specificTermsV1(tokenContract), + "\n", + "Token Contract: ", + Strings.toHexString(address(tokenContract)) + ) + );Note: If you adopt this, align any client-side signing and tests.
test/Forking/Tokenizer14UpgradeForkTest.t.sol (3)
5-5: Remove unused console import and unused SafeERC20Upgradeable using.They aren’t used and may trigger lints.
-import { console } from "forge-std/console.sol"; @@ - using SafeERC20Upgradeable for IPToken;Also applies to: 24-25
80-85: Brittle assert on permissioner address.Hard-coding an address can make the fork test fragile if deployment configs shift. If feasible, assert nonzero and code size, or compare to a sourced constant used in the rollout script.
- assertEq(address(tokenizer14.permissioner()), 0xC837E02982992B701A1B5e4E21fA01cEB0a628fA); + address perm = address(tokenizer14.permissioner()); + assertTrue(perm != address(0)); + assertGt(perm.code.length, 0);
405-414: Also assert Tokenizer disallows issue/cap for wrapped tokens.You check direct calls on WrappedIPToken revert. Add coverage that Tokenizer routes reject (if that’s the intended behavior).
If Tokenizer.issue/cap accept the IIPToken/wrapper type:
vm.expectRevert(); // WrappedIPToken should not allow issue/cap operations WrappedIPToken(address(wrappedToken)).issue(alice, 1000 ether); @@ WrappedIPToken(address(wrappedToken)).cap(); + + // And via Tokenizer + vm.expectRevert(); + tokenizer14.issue(IIPToken(address(wrappedToken)), 1000 ether, alice); + vm.expectRevert(); + tokenizer14.cap(IIPToken(address(wrappedToken)));Adjust types to match the actual Tokenizer API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/test.yml(1 hunks)README.md(1 hunks)foundry.toml(1 hunks)script/dev/Ipnft.s.sol(1 hunks)script/dev/Synthesizer.s.sol(0 hunks)script/dev/Tokenizer.s.sol(3 hunks)script/prod/RolloutTokenizerV13.s.sol(0 hunks)script/prod/RolloutTokenizerV14.s.sol(1 hunks)src/IIPToken.sol(1 hunks)src/IPToken.sol(5 hunks)src/Permissioner.sol(5 hunks)src/Tokenizer.sol(7 hunks)src/WrappedIPToken.sol(1 hunks)src/helpers/test-upgrades/IPToken13.sol(1 hunks)src/helpers/test-upgrades/Tokenizer13.sol(5 hunks)test/CrowdSalePermissioned.t.sol(1 hunks)test/Forking/Tokenizer13UpgradeForkTest.t.sol(0 hunks)test/Forking/Tokenizer14UpgradeForkTest.t.sol(1 hunks)test/Mintpass.t.sol(0 hunks)test/Permissioner.t.sol(1 hunks)test/Tokenizer.t.sol(3 hunks)test/TokenizerWrapped.t.sol(1 hunks)
💤 Files with no reviewable changes (4)
- test/Forking/Tokenizer13UpgradeForkTest.t.sol
- script/prod/RolloutTokenizerV13.s.sol
- test/Mintpass.t.sol
- script/dev/Synthesizer.s.sol
🚧 Files skipped from review as they are similar to previous changes (7)
- script/prod/RolloutTokenizerV14.s.sol
- src/IIPToken.sol
- src/WrappedIPToken.sol
- README.md
- test/CrowdSalePermissioned.t.sol
- .github/workflows/test.yml
- src/helpers/test-upgrades/IPToken13.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-09T14:07:29.973Z
Learnt from: elmariachi111
PR: moleculeprotocol/IPNFT#170
File: script/prod/RolloutV25Sale.s.sol:24-24
Timestamp: 2025-01-09T14:07:29.973Z
Learning: In the IPNFT project, TimelockedToken contract serves as a template for minimal clones and is intentionally initialized with address(0) as it has no real state. The actual token address is set when clones are created.
Applied to files:
src/helpers/test-upgrades/Tokenizer13.sol
🪛 Gitleaks (8.28.0)
test/Forking/Tokenizer14UpgradeForkTest.t.sol
[high] 35-35: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (30)
foundry.toml (2)
7-9: Sanity-check gas snapshots after optimizer/viaIR change.
optimizer = true / optimizer_runs = 200 / via_ir = true (foundry.toml lines 7–9) will shift gas and possibly code size — rebuild and refresh gas snapshots/reports locally:forge clean && forge build
forge test --gas-reportif using snapshots:
forge snapshot
7-9: viaIR=true on solc 0.8.18 — confirm it's safe for inline assembly & upgrades
- foundry.toml currently: solc_version = "0.8.18", optimizer = true, optimizer_runs = 200, via_ir = true.
- Inline assembly present in production: src/Tokenizer.sol (assembly { extcodesize(...) }) — tests: test/RecoverSignatures.t.sol (assembly). Upgrade-related code exists under src/helpers/test-upgrades/ and test/IPNFTUpgrades.t.sol.
- Action: run a full compile + test run (including upgrade/fork scenarios) with viaIR enabled and verify bytecode/behavior; if any miscompilation or behavioral regressions appear, disable via_ir or pin to a known-good compiler/build.
- Optional: for deterministic bytecode/addresses add to foundry.toml near optimizer block:
evm_version = "paris"
bytecode_hash = "none"script/dev/Ipnft.s.sol (1)
94-94: LGTM: listing uses token1Id.Clear and consistent with the new two-mint flow.
test/Permissioner.t.sol (1)
6-8: Imports aligned with IIPToken surface.Switching Metadata to
IIPToken.solwhile keepingIPTokenusage is appropriate sinceIPTokenimplementsIIPToken.Confirm
TermsAcceptedPermissionermethods acceptIIPTokenso calls withIPTokenremain type-compatible at compile time.test/Tokenizer.t.sol (4)
20-22: Imports updated to IIPToken.Metadata and IPToken.Looks consistent with the interface split.
26-26: Pure override is acceptable.Overriding a
viewwithpureis fine (more restrictive) and makes the intent explicit in this test helper.
221-221: Minor: Dropping the unused var is fine.The call side-effects are preserved; removing the unused capture reduces noise.
298-317: Good addition: covers ownership transfer semantics.Solid, focused assertions on who can issue pre/post IPNFT transfer.
script/dev/Tokenizer.s.sol (4)
8-13: New imports for IIPToken/WrappedIPToken are correct.Matches the new wrapping flow.
31-33: Wiring WrappedIPToken implementation is correct.Setter call and logging look good.
Also applies to: 37-37
57-62: Helper to sign terms is clear and reusable.Good encapsulation.
68-78: Attach + mint flow looks correct.Attaches existing ERC20 for #1, mints new IPT for #2, and logs both addresses.
Also applies to: 80-82
test/TokenizerWrapped.t.sol (6)
19-36: Good minimal invalid token mock.Sufficient surface to trigger decimals/code checks.
62-83: Setup is sound.Proxy wiring, implementations, and reservation look correct.
98-109: Invalid contract checks are appropriate.Zero address and EOA both covered.
110-119: Invalid decimals test is on point.Covers the >18 guard.
120-134: Wrapped properties validated correctly.Casts and assertions make sense.
136-150: Behavioral guards on wrapped IPT are enforced.Revert messages checked for issue/cap. Good.
src/Tokenizer.sol (3)
39-42: Events for wrapping and implementation updates look right.Emitting original ERC20 in TokensCreated is a good choice for subgraphs.
46-48: Mapping type change to IIPToken: verify storage compatibility.Type change is address-compatible, but please double-check no storage reordering occurred in prior versions that would shift slots.
247-274: Token contract validation looks solid.Covers zero, code size, and decimals bound with safe try/catch.
src/IPToken.sol (4)
4-4: Upgradeability + interface conformance look good.Inheriting
ERC20Upgradeableand implementingIIPTokenwith proper overrides is correct.Also applies to: 23-23
34-41: Initialize updates are correct.Switch to
__ERC20_init(name_, symbol_)and externalized metadata assignment looks good.
58-72: Overrides are precise.Explicitly delegating to
ERC20UpgradeableforbalanceOf/name/symbol/decimalsavoids ambiguity.
124-125: URI external_url updated.Switch to molecule.xyz is fine.
src/Permissioner.sol (1)
35-36: Confirm event emits the original ERC20 (not the wrapper) as per PR goal.
emit TermsAccepted(address(tokenContract), ...)will log the passed contract’s address. If callers pass the WrappedIPToken, subgraphs will index the wrapper, not the underlying ERC20. If the requirement is to reference the original ERC20, ensure the permissioner gets or derives the underlying and emits that instead.If
IIPToken(or the wrapper) exposes the underlying token (e.g.,underlying()), modify the emission accordingly. Otherwise, document that the tokenizer callsacceptwith the underlying for wrapped flows.Also applies to: 51-52
test/Forking/Tokenizer14UpgradeForkTest.t.sol (2)
33-39: FYI: Gitleaks “generic API key” is a false positive here.Hardcoded EOA/contract addresses are public data; there’s no secret exposure.
If you want to silence the tool, add a repo-level allowlist for these patterns in .gitleaks.toml.
55-67: Resolved — reinit argument order and reinitializer version are correct. Tokenizer.reinit is declared as reinit(WrappedIPToken _wrappedIpTokenImplementation, IPToken _ipTokenImplementation) public onlyOwner reinitializer(6); the test encodes (wrappedImpl, ipTokenImpl) and matches.src/helpers/test-upgrades/Tokenizer13.sol (2)
8-10: Use IIPToken type at the accept call to match the updated interface.Avoid relying on implicit conversions; import IIPToken and cast at the call site for clarity.
-import { IPToken } from "../../IPToken.sol"; -import { Metadata as TokenMetadata } from "../../IIPToken.sol"; +import { IPToken } from "../../IPToken.sol"; +import { IIPToken, Metadata as TokenMetadata } from "../../IIPToken.sol"; @@ - permissioner.accept(token, _msgSender(), signedAgreement); + permissioner.accept(IIPToken(address(token)), _msgSender(), signedAgreement);Also applies to: 155-156
73-85: Good controller gate and synthesized mapping check.Ensures only the current IPNFT owner can operate and that the token is under this tokenizer’s control.
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/dev/Tokenizer.s.sol (1)
24-26: Initialize the proxy atomically to avoid an uninitialized windowPass initCalldata to the ERC1967Proxy constructor to eliminate the race and save a tx.
Apply this diff:
- Tokenizer tokenizer = Tokenizer(address(new ERC1967Proxy(address(new Tokenizer()), ""))); - IPermissioner permissioner = IPermissioner(vm.envAddress("TERMS_ACCEPTED_PERMISSIONER_ADDRESS")); - tokenizer.initialize(IPNFT(vm.envAddress("IPNFT_ADDRESS")), permissioner); + IPermissioner permissioner = IPermissioner(vm.envAddress("TERMS_ACCEPTED_PERMISSIONER_ADDRESS")); + Tokenizer tokenizer = Tokenizer( + address( + new ERC1967Proxy( + address(new Tokenizer()), + abi.encodeWithSelector( + Tokenizer.initialize.selector, + IPNFT(vm.envAddress("IPNFT_ADDRESS")), + permissioner + ) + ) + ) + );
🧹 Nitpick comments (3)
script/dev/Tokenizer.s.sol (3)
31-33: Wrapped implementation wiring: confirm access control and consider consolidated reinitEnsure setWrappedIPTokenImplementation is properly admin‑gated, and consider using the new reinitializer that accepts both implementations in one call to keep versions in sync.
57-62: Avoid signing with a hardcoded CID inside the helperPass iptAgreementCid into prepareAndSignTerms to prevent accidental signature/data mismatches.
Apply this diff:
- function prepareAndSignTerms(uint256 tokenId) internal returns (bytes memory) { - string memory terms = permissioner.specificTermsV1(Metadata(tokenId, bob, "bafkreigk5dvqblnkdniges6ft5kmuly47ebw4vho6siikzmkaovq6sjstq")); + function prepareAndSignTerms(uint256 tokenId, string memory iptAgreementCid) internal returns (bytes memory) { + string memory terms = permissioner.specificTermsV1(Metadata(tokenId, bob, iptAgreementCid)); (uint8 v, bytes32 r, bytes32 s) = vm.sign(bobPk, ECDSA.toEthSignedMessageHash(abi.encodePacked(terms))); return abi.encodePacked(r, s, v); }
68-77: Single source of truth for CID, stronger token typing, and small typo
- Define iptAgreementCid once and reuse for signing and calls.
- Prefer IERC20 for real tokens over FakeERC20 in scripts.
- Fix comment typo.
Apply this diff:
- bytes memory signedToken1Terms = prepareAndSignTerms(1); - FakeERC20 usdc = FakeERC20(vm.envAddress("USDC_ADDRESS")); + string memory iptAgreementCid = "bafkreigk5dvqblnkdniges6ft5kmuly47ebw4vho6siikzmkaovq6sjstq"; + bytes memory signedToken1Terms = prepareAndSignTerms(1, iptAgreementCid); + IERC20 usdc = IERC20(vm.envAddress("USDC_ADDRESS")); // Attach an already existing token as an IPT - IIPToken token1Contract = tokenizer.attachIpt(1, "bafkreigk5dvqblnkdniges6ft5kmuly47ebw4vho6siikzmkaovq6sjstq", signedToken1Terms, usdc); + IIPToken token1Contract = tokenizer.attachIpt(1, iptAgreementCid, signedToken1Terms, usdc); - bytes memory signedToken2Terms = prepareAndSignTerms(2); + bytes memory signedToken2Terms = prepareAndSignTerms(2, iptAgreementCid); - // Mmint a new IPT + // Mint a new IPT - IPToken token2Contract = - tokenizer.tokenizeIpnft(2, 1_000_000 ether, "MOLE", "bafkreigk5dvqblnkdniges6ft5kmuly47ebw4vho6siikzmkaovq6sjstq", signedToken2Terms); + IPToken token2Contract = + tokenizer.tokenizeIpnft(2, 1_000_000 ether, "MOLE", iptAgreementCid, signedToken2Terms);Also add this import at the top of the file:
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";If attachIpt expects an address rather than IERC20, keep the variable as address instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
foundry.toml(1 hunks)script/dev/Ipnft.s.sol(1 hunks)script/dev/Tokenizer.s.sol(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- script/dev/Ipnft.s.sol
- foundry.toml
🔇 Additional comments (4)
script/dev/Tokenizer.s.sol (4)
8-13: LGTM: interface split and centralized Metadata importUsing IIPToken and re‑exported Metadata keeps terms handling consistent across implementations.
37-37: LGTM: logging wrapped implementation addressUseful for scripts/subgraphs to track upgrades.
80-81: LGTM: clear logs for attached vs. minted IPTsMakes it easy to copy/paste addresses into follow‑up calls.
57-61: RSV (r||s||v) encoding is correctprepareAndSignTerms returns abi.encodePacked(r, s, v); TermsAcceptedPermissioner’s NatSpec and implementation expect abi.encodePacked(r, s, v) and validate via SignatureChecker/ECDSA (which accepts 65‑byte RSV). No change required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
script/DeployTokenizer.s.sol (2)
28-31: BYOT requires wiring the Wrapped IP Token implementation on fresh deploys.This script only sets the IPToken implementation. With BYOT, Tokenizer also needs the WrappedIPToken implementation (per the README’s V14 rollout notes). For a fresh deploy, ensure you either:
- Call the new reinitializer that accepts both implementations, or
- Explicitly set the WrappedIPToken implementation after deploy, similar to how
setIPTokenImplementationis invoked.Otherwise, wrapper-based flows may revert when cloning or referencing the wrapper template.
I can help adapt this script once you confirm the exact reinitializer/setter names on Tokenizer in this branch.
23-27: Remove redundant address() cast around the proxy.The outer
address(...)is unnecessary.Apply this diff:
- Tokenizer tokenizer = Tokenizer( - address( - address(new ERC1967Proxy(address(new Tokenizer()), abi.encodeWithSelector(Tokenizer.initialize.selector, ipnftAddress, permissioner))) - ) - ); + Tokenizer tokenizer = Tokenizer( + address( + new ERC1967Proxy( + address(new Tokenizer()), + abi.encodeWithSelector(Tokenizer.initialize.selector, ipnftAddress, permissioner) + ) + ) + );src/IIPToken.sol (3)
33-36: Fix Natspec for cap() to match behavior (setter, not getter).Docs imply a getter/setter hybrid. The interface defines a setter that may revert on wrappers.
Apply:
- /// @notice Returns or sets the maximum supply cap for this token - /// @dev Implementation may vary - could be a getter or setter depending on context + /// @notice Sets the supply cap (disables further issuance) + /// @dev Implementations should mark the token as capped and may emit an event. Wrappers MUST revert. function cap() external;
2-2: Unify pragma with rest of repo (pin to 0.8.18).Keeps compiler consistency across contracts.
-pragma solidity ^0.8; +pragma solidity 0.8.18;
15-21: Confirm non-inheritance from IERC20 is intentional.Given WrappedIPToken is not itself an ERC20, keeping IIPToken ERC20-agnostic makes sense. Consider a one-liner in the file-level doc stating this intentionally does not extend IERC20.
src/libraries/IPTokenUtils.sol (2)
32-34: Emit numeric supply in JSON (not a quoted string).Avoids type drift and improves downstream parsing.
- '","supply": "', - Strings.toString(supply), - '"}' + '","supply": ', + Strings.toString(supply), + '}'
5-5: Remove unused IERC20 import.-import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";src/WrappedIPToken.sol (1)
2-2: Optional: align pragma with others (0.8.18).-pragma solidity 0.8.18; +pragma solidity 0.8.18; // (no-op if already standardized project-wide)src/Tokenizer.sol (3)
129-135: Update reinit() docstring (now wires implementations; no legacy mapping changes).- /** - * @dev sets legacy IPTs on the tokenized mapping - */ + /** + * @dev sets implementation addresses for IPToken and WrappedIPToken (v1.4 migration hook) + */
183-194: Fix return type in Natspec for attachIpt.Doc says IPToken but function returns IIPToken.
- * @return IPToken a wrapped IPToken that represents the tokenized ipnft for permissioners and carries metadata + * @return IIPToken a wrapped IPToken that represents the tokenized ipnft for permissioners and carries metadata
209-218: Harden event field reads against non‑standard ERC20s (optional).Some ERC20s revert on name/symbol/totalSupply. Use try/catch and fallbacks before emitting.
- emit TokensCreated( + // Defensive reads for non-standard tokens + string memory _name; + string memory _symbol; + uint256 _supply; + try tokenContract.name() returns (string memory n) { _name = n; } catch {} + try tokenContract.symbol() returns (string memory s) { _symbol = s; } catch {} + try tokenContract.totalSupply() returns (uint256 ts) { _supply = ts; } catch {} + + emit TokensCreated( uint256(keccak256(abi.encodePacked(ipnftId))), ipnftId, address(tokenContract), _msgSender(), - tokenContract.totalSupply(), + _supply, agreementCid, - tokenContract.name(), - tokenContract.symbol() + _name, + _symbol );test/Forking/Tokenizer14UpgradeForkTest.t.sol (1)
409-416: Add assertions that WrappedIPToken’s ERC20 mutating methods revert.Guards against accidental forwarding if someone reintroduces it.
// Test that wrapped tokens cannot issue or cap (they should delegate to underlying token) vm.expectRevert(); // WrappedIPToken should not allow issue/cap operations WrappedIPToken(address(wrappedToken)).issue(alice, 1000 ether); vm.expectRevert(); // WrappedIPToken should not allow cap operations WrappedIPToken(address(wrappedToken)).cap(); + + // And ER20-like state-changing methods on the wrapper should revert + address bob = makeAddr("bob"); + vm.startPrank(alice); + vm.expectRevert(); + WrappedIPToken(address(wrappedToken)).transfer(bob, 1 ether); + vm.expectRevert(); + WrappedIPToken(address(wrappedToken)).approve(bob, 1 ether); + vm.expectRevert(); + WrappedIPToken(address(wrappedToken)).transferFrom(alice, bob, 1 ether); + vm.stopPrank();src/IPToken.sol (1)
119-121: Confirm intended “supply” in URI is totalIssued, not current totalSupply.URIs for wrapped tokens use totalSupply; here you pass totalIssued (ever minted). If burns are possible, this diverges. If you prefer “current supply,” switch to totalSupply().
- return IPTokenUtils.generateURI(_metadata, address(this), totalIssued); + return IPTokenUtils.generateURI(_metadata, address(this), totalSupply());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
foundry.toml(2 hunks)script/DeployTokenizer.s.sol(1 hunks)script/prod/RolloutTokenizerV14.s.sol(1 hunks)src/IIPToken.sol(1 hunks)src/IPToken.sol(6 hunks)src/Tokenizer.sol(6 hunks)src/WrappedIPToken.sol(1 hunks)src/libraries/IPTokenUtils.sol(1 hunks)test/Forking/Tokenizer14UpgradeForkTest.t.sol(1 hunks)test/TokenizerWrapped.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/TokenizerWrapped.t.sol
- script/prod/RolloutTokenizerV14.s.sol
- foundry.toml
🔇 Additional comments (4)
script/DeployTokenizer.s.sol (2)
4-4: Confirm BioPriceFeed import necessity.
BioPriceFeedisn’t referenced in this file. If unused, drop the import to keep the script lean.
13-15: LGTM: required deps for proxy/script usage are correctly added.
ERC1967Proxy,forge-std/Script.sol, andconsole.solare all used below; imports look good.src/WrappedIPToken.sol (1)
93-95: LGTM: Centralized URI generation via IPTokenUtils.src/IPToken.sol (1)
33-40: Initialize ERC20 with calldata params (ok).Using calldata here is fine; compiler will copy to memory for __ERC20_init. No action.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
test/Forking/Tokenizer14UpgradeForkTest.t.sol (3)
32-40: Avoid hardcoded sensitive values in test files.The test contains hardcoded mainnet addresses including what appears to be a mainnet deployer address. Based on Foundry fork testing best practices, RPC URLs should be stored in environment variables, and similarly, sensitive addresses should not be hardcoded.
Consider moving these addresses to environment variables or a configuration file:
- address mainnetDeployer = 0x34021576F01275A429163a56908Bd02b43e2B7e1; - address mainnetOwner = 0xCfA0F84660fB33bFd07C369E5491Ab02C449f71B; - address mainnetTokenizer = 0x58EB89C69CB389DBef0c130C6296ee271b82f436; - address mainnetIPNFT = 0xcaD88677CA87a7815728C72D74B4ff4982d54Fc1; + address mainnetDeployer = vm.envAddress("MAINNET_DEPLOYER"); + address mainnetOwner = vm.envAddress("MAINNET_OWNER"); + address mainnetTokenizer = vm.envAddress("MAINNET_TOKENIZER"); + address mainnetIPNFT = vm.envAddress("MAINNET_IPNFT");
123-205: Improve conditional balance checks for robustness.The tests use conditional balance checks (
if (paulhaasBalance >= 100_000 ether)) which might lead to incomplete test coverage if the conditions aren't met. This could result in silent test failures where important functionality isn't actually validated.Consider either:
- Using a more deterministic approach by dealing specific amounts to test accounts
- Adding assertions that the conditions are met to ensure test coverage
uint256 paulhaasBalance = ipt.balanceOf(paulhaas); - if (paulhaasBalance >= 100_000 ether) { + // Ensure we have enough balance for testing + assertTrue(paulhaasBalance >= 100_000 ether, "Insufficient balance for transfer test"); vm.startPrank(paulhaas);Or alternatively, deal funds directly:
+ // Deal tokens directly for consistent testing + vm.store(address(ipt), keccak256(abi.encode(paulhaas, 0)), bytes32(uint256(200_000 ether))); + assertEq(ipt.balanceOf(paulhaas), 200_000 ether);
327-357: Remove duplicate line for cleaner test.Line 346 is identical to line 345, creating unnecessary duplication in the interface compatibility test.
// Compare results from both interfaces (only IP-specific functions available on interface) assertEq(newToken.totalIssued(), interfaceToken.totalIssued()); - assertEq(newToken.totalIssued(), interfaceToken.totalIssued());test/TokenizerWrapped.t.sol (7)
96-99: Strengthen synthesized mapping assertion.Also assert synthesized equals the wrapper address, not just “not the underlying”.
- assertNotEq(address(tokenizer.synthesized(1)), address(erc20)); // the synthesized member tracks the wrapped ipt + assertNotEq(address(tokenizer.synthesized(ipnftId)), address(erc20)); // not the underlying + assertEq(address(tokenizer.synthesized(ipnftId)), address(wrappedImpl)); // equals wrapper
64-85: End the prank in setUp.Clean test hygiene: stop the originalOwner prank at the end of setUp.
ipnft.mintReservation{ value: MINTING_FEE }(originalOwner, reservationId, ipfsUri, DEFAULT_SYMBOL, ""); + vm.stopPrank();
53-57: Remove unused test keys (and bob).alicePk, bob, and bobPk are unused.
- address alice = makeAddr("alice"); - uint256 alicePk; - - address bob = makeAddr("bob"); - uint256 bobPk; + address alice = makeAddr("alice");If you need the keys later, keep them; otherwise, drop the makeAddrAndKey calls in setUp as well.
190-193: Either use or drop the contains helper; consider asserting URI contents.The helper is unused. If the URI encodes agreementCid/ipnftId, assert it to harden the test; else remove the helper.
Option A: use it
string memory uri = wrappedToken.uri(); - assertTrue(bytes(uri).length > 0); + assertTrue(bytes(uri).length > 0); + assertTrue(contains(uri, agreementCid)); + assertTrue(contains(uri, vm.toString(ipnftId)));Option B: delete contains(...)
- // Helper function to check if a string contains a substring - function contains(string memory source, string memory search) internal pure returns (bool) { - bytes memory sourceBytes = bytes(source); - bytes memory searchBytes = bytes(search); - if (searchBytes.length > sourceBytes.length) { - return false; - } - for (uint256 i = 0; i <= sourceBytes.length - searchBytes.length; i++) { - bool found = true; - for (uint256 j = 0; j < searchBytes.length; j++) { - if (sourceBytes[i + j] != searchBytes[j]) { - found = false; - break; - } - } - if (found) { - return true; - } - } - return false; - }Also applies to: 218-244
113-121: Add positive test for sub-18 decimals (e.g., 6).You only cover >18 as invalid. Add a 6-decimals token case to prove acceptance (USDC-like).
If FakeERC20 supports configurable decimals, mint a 6-dec token and attach; else, add a small helper:
contract FakeERC20With6Decimals { function decimals() external pure returns (uint8) { return 6; } function name() external pure returns (string memory) { return "SixDec"; } function symbol() external pure returns (string memory) { return "SIX"; } function totalSupply() external pure returns (uint256) { return 1_000_000 * 1e6; } }Then assert attach works and balances/totalIssued scale correctly.
101-111: Consider testing event emission (original token vs wrapper).Per PR: event should reference the original ERC20. Add expectEmit to assert args.
Example:
vm.expectEmit(); emit IptAttached(ipnftId, address(erc20), /* ... other fields ... */); tokenizer.attachIpt(ipnftId, agreementCid, "", erc20);Adjust names/signatures to match Tokenizer’s event.
147-152: Fix revert reason typo and prefer selectors/custom errors.WrappedIPToken.sol contains the string reverts with a typo ("cappping" at lines 71–78). Fix the spelling in the contract and update tests to match, or (recommended) replace these string reverts with a custom error and assert by selector in tests.
Quick fix (test):
- vm.expectRevert("WrappedIPToken: read-only wrapper - use underlying token for cappping"); + vm.expectRevert("WrappedIPToken: read-only wrapper - use underlying token for capping");Recommended (tests -> selector):
- vm.expectRevert("WrappedIPToken: read-only wrapper - use underlying token for minting"); + vm.expectRevert(WrappedIPToken.ReadOnly.selector); tokenContract.issue(alice, 1000); - vm.expectRevert("WrappedIPToken: read-only wrapper - use underlying token for cappping"); + vm.expectRevert(WrappedIPToken.ReadOnly.selector); tokenContract.cap();Locations: WrappedIPToken.sol:71–78; tests: test/TokenizerWrapped.t.sol:147–152 and 161–167.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/WrappedIPToken.sol(1 hunks)test/Forking/Tokenizer14UpgradeForkTest.t.sol(1 hunks)test/TokenizerWrapped.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/WrappedIPToken.sol
🧰 Additional context used
🪛 Gitleaks (8.28.0)
test/Forking/Tokenizer14UpgradeForkTest.t.sol
[high] 39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
test/Forking/Tokenizer14UpgradeForkTest.t.sol (8)
54-57: LGTM!The fork setup correctly uses environment variables for the RPC URL and pins to a specific block number for reproducible tests. This follows Foundry best practices.
59-78: LGTM!The upgrade flow correctly implements the V14 upgrade process:
- Deploys new implementations with proper access controls
- Uses the new
reinitsignature with both IPToken and WrappedIPToken implementations- Sets up the test environment appropriately
80-99: LGTM!Comprehensive validation of the upgrade and initialization safeguards. The test correctly verifies:
- New implementations are properly wired
- Initialization can only happen once for all contracts
- Both regular users and owners are prevented from re-initialization
101-121: LGTM!Excellent validation of migration behavior. The test confirms:
- Old tokens are properly migrated and accessible
- Prevents duplicate tokenization of already-tokenized IPNFTs
- Enforces proper ownership controls
207-256: LGTM!Excellent test validating the new tokenizer-mediated issuance functionality. This correctly demonstrates:
- Legacy tokens maintain backward compatibility
- New IPNFT owners can issue tokens through the tokenizer
- Proper access control enforcement for both direct and tokenizer-mediated operations
258-294: LGTM!Comprehensive validation of the new IIPToken interface functionality. The test properly verifies:
- Enhanced metadata access through the new interface
- URI generation functionality
- Proper interface implementation for new tokens
296-325: LGTM!Good validation of legacy token interoperability ensuring the upgrade maintains backward compatibility while still allowing tokenizer-mediated operations.
359-426: LGTM!Excellent comprehensive test for the new "bring your own token" functionality. The test properly validates:
- ERC20 attachment and wrapping mechanism
- WrappedIPToken interface compliance and metadata
- Prevention of multiple token attachments to the same IPNFT
- Proper delegation and access control for wrapped tokens
0xmme
force-pushed
the
feat/bring-your-own-ipt
branch
from
88feb64 to
b6adf2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/Forking/Tokenizer14UpgradeForkTest.t.sol (1)
411-415: Use the actual revert messages for specificity instead of generic expectRevert().The original diff suggests using a non-existent custom error selector. In reality, WrappedIPToken uses plain string reverts. Specify the actual revert messages for better test clarity:
- vm.expectRevert(); // WrappedIPToken should not allow issue/cap operations + vm.expectRevert("WrappedIPToken: read-only wrapper - use underlying token for minting"); WrappedIPToken(address(wrappedToken)).issue(alice, 1000 ether); - vm.expectRevert(); // WrappedIPToken should not allow cap operations + vm.expectRevert("WrappedIPToken: read-only wrapper - use underlying token for cappping"); WrappedIPToken(address(wrappedToken)).cap();This ensures the test catches only the intended reverts and prevents masking unintended failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/Tokenizer.sol(6 hunks)src/WrappedIPToken.sol(1 hunks)test/Forking/Tokenizer14UpgradeForkTest.t.sol(1 hunks)test/TokenizerWrapped.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/WrappedIPToken.sol
- test/TokenizerWrapped.t.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Smart contract tests
🔇 Additional comments (2)
src/Tokenizer.sol (2)
253-276: Validation logic is well-designed.The token contract validation covers the essential checks:
- Non-zero address
- Contract code exists (not an EOA)
- Decimals are within a reasonable range (0-18)
- Graceful handling of non-standard tokens via try-catch
231-242: Verify that issue/cap are intentionally limited to IPToken.The
issueandcapfunctions only acceptIPTokenrather thanIIPToken, which means they cannot be called for wrapped tokens. This appears intentional since wrapped tokens delegate to the underlying ERC20, but please confirm this design decision is documented and understood.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
28-37: Minor: Inconsistent capitalization of "iptoken" vs. "IPToken".Lines 37 and 66 reference "wrapped iptoken implementation" while most other lines use "iptoken implementation" (lines 28, 33, 62, 64). Consider standardizing capitalization throughout the document (e.g., "Wrapped IPToken" to match "IPToken"). This is a very minor consistency improvement.
Also applies to: 62-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
README.md
[high] 33-33: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 35-35: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 37-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 60-60: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 64-64: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 66-66: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[grammar] ~37-~37: Ensure spelling is correct
Context: ...A64e3429aB852d3CB31AD7aa06997A wrapped iptoken implementation: 0x0ca5f50a8a59a59ef8c8d...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~66-~66: Ensure spelling is correct
Context: ...7af053f42425a03f68da9920287d5b wrapped iptoken implementation: 0xa3b844450e31e541e6042...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (2)
README.md (2)
33-37: Gitleaks false positives: Contract addresses are not credentials.The 8 static analysis warnings from Gitleaks flagging "Generic API Key" detections at these lines are false positives. These are public Ethereum/Sepolia mainnet contract implementation addresses, not API keys or secrets, and require no remediation.
Also applies to: 60-60, 64-64, 66-66
113-119: Verify the hex-encoded parameters in the upgrade command are correct.The cast send command at line 118 includes inline hex-encoded parameters that are not immediately transparent. While the comment at line 117 attempts to document the structure
(address, bytes)(0xNewImpl, 0xNewWrappedIPTokenImpl 0xNewIPTokenImpl), the actual hex payload0x8b3d19bb...should be verified for correctness. Ensure the encoding matches the expectedupgradeToAndCall(address,bytes)signature and that the two implementation addresses (new IPToken and Wrapped IPToken) are correctly included in the encoded calldata.
0xmme
force-pushed
the
feat/bring-your-own-ipt
branch
from
06e5435 to
8ec9be7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
112-117: Upgrade instructions are clear, but specify deployment target for context.The Tokenizer 1.4 upgrade commands are provided, but they reference a generic RPC URL. Consider adding a comment to clarify this is specifically for mainnet, or note that the script (
RolloutTokenizerV14.s.sol) should be adjusted for testnet deployments.### upgrading to Tokenizer 1.4 +// Mainnet upgrade only - adjust RPC and deployment addresses for other networks forge script --private-key=$PRIVATE_KEY --rpc-url=$RPC_URL script/prod/RolloutTokenizerV14.s.sol --broadcast
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(4 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
README.md
[high] 20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 33-33: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 59-59: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 63-63: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[grammar] ~36-~36: Ensure spelling is correct
Context: ...A64e3429aB852d3CB31AD7aa06997A wrapped iptoken implementation: 0x0ca5f50a8a59a59ef8c8d...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~65-~65: Ensure spelling is correct
Context: ...7af053f42425a03f68da9920287d5b wrapped iptoken implementation: 0xa3b844450e31e541e6042...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~121-~121: Use a hyphen to join words.
Context: ...so upcoming instances can be verified on chain. --- ## Prerequisites To work w...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
24-24: Bare URL used
(MD034, no-bare-urls)
25-25: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
README.md (1)
20-20: Documentation update accurately reflects v1.4 tokenizer and wrapped token implementations.The README has been updated comprehensively to document the new 1.4 implementations for both mainnet and Sepolia:
- New tokenizer, IPToken, and WrappedIPToken implementation addresses are clearly listed
- Timelocked token implementations externalized with dedicated entries
- Upgrade instructions and deployment guidance are included
- Subgraph and contract address tables restructured to reflect the new contracts
The changes align well with the PR objectives to support "Bring Your Own IPT" functionality.
Also applies to: 27-36, 59-66
0xmme
force-pushed
the
feat/bring-your-own-ipt
branch
from
8ec9be7 to
20e8908
Compare
Summary by CodeRabbit
New Features
Documentation
Tests
Chores