Conversation
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/token/ERC7984/extensions/ERC7984Metadata.sol (1)
13-17: Consider adding an internal_setContractURIsetter for post-deployment URI updates.The URI is currently write-once (constructor only). Subclasses that need to update it (e.g., after an IPFS migration) must override
contractURI()entirely. Adding an internal setter following the same pattern as OpenZeppelin'sERC6909ContentURI._setContractURIprovides composable extensibility without exposing an unguarded public surface.✨ Suggested addition
constructor(string memory contractURI_) { _contractURI = contractURI_; } +/// `@dev` Sets the contract URI and emits a {ContractURIUpdated} event. +function _setContractURI(string memory newContractURI) internal virtual { + _contractURI = newContractURI; + emit ContractURIUpdated(); +} +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/token/ERC7984/extensions/ERC7984Metadata.sol` around lines 13 - 17, The contract stores the contract URI in the private variable _contractURI and only sets it in the constructor, preventing subclasses from updating it; add an internal setter named _setContractURI(string memory) that updates _contractURI (mirroring OpenZeppelin's ERC6909ContentURI._setContractURI) so subclasses can call _setContractURI(...) to change the value post-deployment while keeping the setter internal and preserving the existing public contractURI() accessor.contracts/interfaces/IERC7984Metadata.sol (1)
8-10:ContractURIUpdated()event missing from the ERC-7572 interface.The canonical ERC-7572 interface is
interface IERC7572 { function contractURI() external view returns (string memory); event ContractURIUpdated(); }, and theContractURIUpdated()event is specified to help offchain indexers know when to refetch the metadata.Its absence from
IERC7984Metadatameans the interface doesn't fully conform to ERC-7572, and consumers building on this interface won't have the standard event hook. Even ifERC7984Metadatatoday only sets the URI at construction, declaring the event here (and emitting it from a future_setContractURIsetter) follows the ERC-7572 spec and aligns with how OpenZeppelin implements the same pattern inERC6909ContentURI.✨ Suggested addition
interface IERC7984Metadata is IERC7984 { + /// `@dev` Emitted when the contract URI is updated. See https://eips.ethereum.org/EIPS/eip-7572[ERC-7572]. + event ContractURIUpdated(); + /// `@dev` Returns the contract URI. Should be formatted as described in https://eips.ethereum.org/EIPS/eip-7572[ERC-7572]. function contractURI() external view returns (string memory); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/interfaces/IERC7984Metadata.sol` around lines 8 - 10, Add the missing ERC-7572 event to the interface: declare the ContractURIUpdated (or ContractURIUpdated()) event inside IERC7984Metadata alongside the existing contractURI() function so the interface fully conforms to ERC-7572; update implementing contracts (e.g., ERC7984Metadata) to emit ContractURIUpdated when a future _setContractURI or similar setter changes the URI.test/token/ERC7984/ERC7984.test.ts (1)
48-58: Consider adding a negativeERC7984Metadatainterface check.The ERC165 block already asserts
ERC7984ERC20WrapperandERC7984RWAreturnfalse. Since the explicit goal of this PR is to gatecontractURI()behind an opt-in extension, a matching negative assertion forINTERFACE_IDS.ERC7984Metadatawould close the loop and prevent regressions.🛡️ Suggested addition
await expect(this.token.supportsInterface(INTERFACE_IDS.ERC7984)).to.eventually.be.true; await expect(this.token.supportsInterface(INTERFACE_IDS.ERC7984ERC20Wrapper)).to.eventually.be.false; await expect(this.token.supportsInterface(INTERFACE_IDS.ERC7984RWA)).to.eventually.be.false; +await expect(this.token.supportsInterface(INTERFACE_IDS.ERC7984Metadata)).to.eventually.be.false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/token/ERC7984/ERC7984.test.ts` around lines 48 - 58, Add a negative ERC7984Metadata interface check in the ERC165 tests: in the describe('ERC165') block add an assertion using this.token.supportsInterface(INTERFACE_IDS.ERC7984Metadata) and expect it to.eventually.be.false (matching the style of the existing supportsInterface checks), so the test explicitly verifies that INTERFACE_IDS.ERC7984Metadata is not supported by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/interfaces/IERC7984Metadata.sol`:
- Line 3: The pragma in IERC7984Metadata.sol is set to ^0.8.24 which is
inconsistent with the rest of the PR; update the Solidity pragma in that
interface (IERC7984Metadata) to ^0.8.27 so it matches the other contracts and
avoid compiler-version mismatches, then recompile/tests to verify compatibility
with the implementing contracts.
---
Nitpick comments:
In `@contracts/interfaces/IERC7984Metadata.sol`:
- Around line 8-10: Add the missing ERC-7572 event to the interface: declare the
ContractURIUpdated (or ContractURIUpdated()) event inside IERC7984Metadata
alongside the existing contractURI() function so the interface fully conforms to
ERC-7572; update implementing contracts (e.g., ERC7984Metadata) to emit
ContractURIUpdated when a future _setContractURI or similar setter changes the
URI.
In `@contracts/token/ERC7984/extensions/ERC7984Metadata.sol`:
- Around line 13-17: The contract stores the contract URI in the private
variable _contractURI and only sets it in the constructor, preventing subclasses
from updating it; add an internal setter named _setContractURI(string memory)
that updates _contractURI (mirroring OpenZeppelin's
ERC6909ContentURI._setContractURI) so subclasses can call _setContractURI(...)
to change the value post-deployment while keeping the setter internal and
preserving the existing public contractURI() accessor.
In `@test/token/ERC7984/ERC7984.test.ts`:
- Around line 48-58: Add a negative ERC7984Metadata interface check in the
ERC165 tests: in the describe('ERC165') block add an assertion using
this.token.supportsInterface(INTERFACE_IDS.ERC7984Metadata) and expect it
to.eventually.be.false (matching the style of the existing supportsInterface
checks), so the test explicitly verifies that INTERFACE_IDS.ERC7984Metadata is
not supported by default.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
contracts/interfaces/IERC7984.solcontracts/interfaces/IERC7984Metadata.solcontracts/mocks/docs/ERC7984MintableBurnable.solcontracts/mocks/token/ERC7984ERC20WrapperMock.solcontracts/mocks/token/ERC7984FreezableMock.solcontracts/mocks/token/ERC7984MetadataMock.solcontracts/mocks/token/ERC7984Mock.solcontracts/mocks/token/ERC7984ObserverAccessMock.solcontracts/mocks/token/ERC7984ReentrantMock.solcontracts/mocks/token/ERC7984RwaMock.solcontracts/mocks/token/ERC7984VotesMock.solcontracts/token/ERC7984/ERC7984.solcontracts/token/ERC7984/extensions/ERC7984Metadata.soltest/finance/ERC7821WithExecutor.test.tstest/finance/VestingWalletCliffConfidential.test.tstest/finance/VestingWalletConfidential.behavior.tstest/finance/VestingWalletConfidential.test.tstest/finance/VestingWalletConfidentialFactory.test.tstest/helpers/interface.tstest/token/ERC7984/ERC7984.test.tstest/token/ERC7984/ERC7984Votes.test.tstest/token/ERC7984/extensions/ERC7984ERC20Wrapper.test.tstest/token/ERC7984/extensions/ERC7984Freezable.test.tstest/token/ERC7984/extensions/ERC7984Metadata.test.tstest/token/ERC7984/extensions/ERC7984ObserverAccess.test.tstest/token/ERC7984/extensions/ERC7984Omnibus.test.tstest/token/ERC7984/extensions/ERC7984Restricted.test.tstest/token/ERC7984/extensions/ERC7984Rwa.test.ts
💤 Files with no reviewable changes (2)
- test/token/ERC7984/extensions/ERC7984Freezable.test.ts
- contracts/interfaces/IERC7984.sol
arr00
changed the title
contractIURI() to extensioncontractURI() to extension
1 participant
The
contractURI()function seems like it would belong best in an extension.Summary by CodeRabbit
Breaking Changes
New Features