Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR adds a new Shape Registry System across the codebase: ABI, TypeScript/ethers contract bindings, client mappings, deployment bytecodes/mappings, a Solidity library (LibShapeRegistry) implementing registry logic, and an admin-only system contract (_ShapeRegistrySystem) exposing create/remove/flag/owner-comp operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant ShapeSystem as _ShapeRegistrySystem
participant Lib as LibShapeRegistry
participant Comps as Components
Admin->>ShapeSystem: execute(bytes arguments)
activate ShapeSystem
ShapeSystem->>ShapeSystem: decode(type_, description)
ShapeSystem->>Lib: create(comps, type_, description)
activate Lib
Lib->>Lib: genID(type_)
Lib->>Comps: write Type, Name, Description, IsRegistry, IdHolder
Lib-->>ShapeSystem: id
deactivate Lib
ShapeSystem-->>Admin: return encoded bytes
deactivate ShapeSystem
Admin->>ShapeSystem: addOwnerComp(type_, ownerCompID)
activate ShapeSystem
ShapeSystem->>Lib: get(comps, type_)
Lib-->>ShapeSystem: id
ShapeSystem->>Lib: addOwnerComp(comps, id, ownerCompID)
Lib->>Comps: set IdHolder
deactivate ShapeSystem
Admin->>ShapeSystem: remove(type_)
activate ShapeSystem
ShapeSystem->>Lib: get(comps, type_)
Lib-->>ShapeSystem: id
ShapeSystem->>Lib: remove(comps, id)
Lib->>Comps: clear Type, Name, Description, IsRegistry, IdHolder, flags
deactivate ShapeSystem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧬 Code graph analysis (1)packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts (1)
🪛 Biome (2.1.2)packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts[error] 172-172: An empty interface is equivalent to {}. Safe fix: Use a type alias instead. (lint/suspicious/noEmptyInterface) 🔇 Additional comments (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/client/abi/_ShapeRegistrySystem.json(1 hunks)packages/client/types/SystemAbis.mjs(2 hunks)packages/client/types/SystemMappings.ts(2 hunks)packages/client/types/SystemTypes.ts(2 hunks)packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts(1 hunks)packages/client/types/ethers-contracts/index.ts(2 hunks)packages/contracts/deploy.json(1 hunks)packages/contracts/deployment/contracts/mappings/SystemBytecodes.ts(2 hunks)packages/contracts/deployment/contracts/mappings/SystemMappings.ts(2 hunks)packages/contracts/src/libraries/LibShapeRegistry.sol(1 hunks)packages/contracts/src/systems/_ShapeRegistrySystem.sol(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/contracts/deployment/contracts/mappings/SystemBytecodes.ts (2)
packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts (1)
_ShapeRegistrySystem(173-383)packages/client/types/ethers-contracts/index.ts (1)
_ShapeRegistrySystem(201-201)
packages/client/types/SystemAbis.mjs (2)
packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts (1)
_ShapeRegistrySystem(173-383)packages/client/types/ethers-contracts/index.ts (1)
_ShapeRegistrySystem(201-201)
packages/client/types/SystemTypes.ts (2)
packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts (1)
_ShapeRegistrySystem(173-383)packages/client/types/ethers-contracts/index.ts (1)
_ShapeRegistrySystem(201-201)
packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts (1)
packages/client/types/ethers-contracts/index.ts (1)
_ShapeRegistrySystem(201-201)
🪛 Biome (2.1.2)
packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts
[error] 166-166: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
| export namespace SystemDeprecatedEvent { | ||
| export type InputTuple = []; | ||
| export type OutputTuple = []; | ||
| export interface OutputObject {} | ||
| export type Event = TypedContractEvent<InputTuple, OutputTuple, OutputObject>; | ||
| export type Filter = TypedDeferredTopicFilter<Event>; | ||
| export type Log = TypedEventLog<Event>; | ||
| export type LogDescription = TypedLogDescription<Event>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace empty interface with type alias
Biome flags empty interfaces as errors (lint/suspicious/noEmptyInterface). Switch this to a type alias so the generated typings pass lint.
-export namespace SystemDeprecatedEvent {
- export type InputTuple = [];
- export type OutputTuple = [];
- export interface OutputObject {}
+export namespace SystemDeprecatedEvent {
+ export type InputTuple = [];
+ export type OutputTuple = [];
+ export type OutputObject = Record<string, never>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export namespace SystemDeprecatedEvent { | |
| export type InputTuple = []; | |
| export type OutputTuple = []; | |
| export interface OutputObject {} | |
| export type Event = TypedContractEvent<InputTuple, OutputTuple, OutputObject>; | |
| export type Filter = TypedDeferredTopicFilter<Event>; | |
| export type Log = TypedEventLog<Event>; | |
| export type LogDescription = TypedLogDescription<Event>; | |
| } | |
| export namespace SystemDeprecatedEvent { | |
| export type InputTuple = []; | |
| export type OutputTuple = []; | |
| export type OutputObject = Record<string, never>; | |
| export type Event = TypedContractEvent<InputTuple, OutputTuple, OutputObject>; | |
| export type Filter = TypedDeferredTopicFilter<Event>; | |
| export type Log = TypedEventLog<Event>; | |
| export type LogDescription = TypedLogDescription<Event>; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 166-166: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🤖 Prompt for AI Agents
In packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts around lines
163 to 171, the generated empty interface "OutputObject {}" triggers the lint
rule lint/suspicious/noEmptyInterface; replace that empty interface with a type
alias (e.g., "export type OutputObject = {}") so the typings satisfy the linter
while keeping the same shape and exported name.
| function execute(bytes memory arguments) public onlyAdmin(components) returns (bytes memory) { | ||
| (string memory type_, string memory description) = abi.decode(arguments, (string, string)); | ||
| LibShapeRegistry.create(components, type_, description); | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Add the missing override specifier.
System.execute(bytes) is declared virtual; without override this implementation fails to compile because the abstract base method remains unresolved. Please tag the function as an override so the contract builds.
- function execute(bytes memory arguments) public onlyAdmin(components) returns (bytes memory) {
+ function execute(bytes memory arguments) public override onlyAdmin(components) returns (bytes memory) {🤖 Prompt for AI Agents
In packages/contracts/src/systems/_ShapeRegistrySystem.sol around lines 16 to
20, the execute(bytes) implementation is missing the required override
specifier; update the function signature to include the override keyword (e.g.,
function execute(bytes memory arguments) public override onlyAdmin(components)
returns (bytes memory)) so it correctly overrides the virtual
System.execute(bytes) declaration and the contract will compile.
chlwys
force-pushed
the
ap.shape_registry_system
branch
from
6ddc623 to
58c3346
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/contracts/src/systems/_ShapeRegistrySystem.sol (1)
16-20: Add the missingoverridespecifier.The
executefunction must include theoverridekeyword to properly override the virtualSystem.execute(bytes)method. Without it, the contract will fail to compile.Apply this diff:
- function execute(bytes memory arguments) public onlyAdmin(components) returns (bytes memory) { + function execute(bytes memory arguments) public override onlyAdmin(components) returns (bytes memory) {packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts (1)
169-177: Replace empty interface with type alias to satisfy linter.The empty
OutputObjectinterface triggers Biome'slint/suspicious/noEmptyInterfaceerror. Replace it with a type alias.Apply this diff:
export namespace SystemDeprecatedEvent { export type InputTuple = []; export type OutputTuple = []; - export interface OutputObject {} + export type OutputObject = Record<string, never>; export type Event = TypedContractEvent<InputTuple, OutputTuple, OutputObject>; export type Filter = TypedDeferredTopicFilter<Event>; export type Log = TypedEventLog<Event>; export type LogDescription = TypedLogDescription<Event>; }
🧹 Nitpick comments (1)
packages/contracts/src/libraries/LibShapeRegistry.sol (1)
41-43: Consider guarding against overwriting existing owner component.The function allows silently overwriting an existing owner component ID. While the system layer validates existence, consider whether replacing an owner component should emit a warning or require explicit confirmation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/client/abi/_ShapeRegistrySystem.json(1 hunks)packages/client/types/SystemAbis.mjs(2 hunks)packages/client/types/SystemMappings.ts(2 hunks)packages/client/types/SystemTypes.ts(2 hunks)packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts(1 hunks)packages/client/types/ethers-contracts/index.ts(2 hunks)packages/contracts/deploy.json(1 hunks)packages/contracts/deployment/contracts/mappings/SystemBytecodes.ts(2 hunks)packages/contracts/deployment/contracts/mappings/SystemMappings.ts(2 hunks)packages/contracts/src/libraries/LibShapeRegistry.sol(1 hunks)packages/contracts/src/systems/_ShapeRegistrySystem.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/contracts/deployment/contracts/mappings/SystemMappings.ts
- packages/contracts/deployment/contracts/mappings/SystemBytecodes.ts
- packages/client/types/SystemAbis.mjs
- packages/contracts/deploy.json
- packages/client/types/SystemMappings.ts
- packages/client/types/SystemTypes.ts
- packages/client/types/ethers-contracts/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts (1)
packages/client/types/ethers-contracts/index.ts (1)
_ShapeRegistrySystem(201-201)
🪛 Biome (2.1.2)
packages/client/types/ethers-contracts/_ShapeRegistrySystem.ts
[error] 172-172: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (5)
packages/contracts/src/libraries/LibShapeRegistry.sol (5)
45-47: LGTM!The flag addition logic correctly delegates to
LibFlag.setFullwith the registry type and flag type.
49-59: LGTM! Past issue has been addressed.The
removefunction now properly clears theIsRegistryComponentflag (line 55), addressing the previous review concern. All associated components and flags are cleaned up correctly.
64-66: LGTM!The instance checker correctly delegates to
LibEntityType.isShape.
71-74: LGTM!The getter correctly uses the sentinel value
0to indicate non-existent registries, which aligns with the system's existence checks.
79-81: LGTM!The deterministic ID generation using a namespaced prefix is appropriate for collision avoidance.
| function create( | ||
| IUintComp comps, | ||
| string memory type_, | ||
| string memory description | ||
| ) internal returns (uint256 id) { | ||
| id = genID(type_); | ||
| LibEntityType.set(comps, id, "SHAPE_REGISTRY"); | ||
| IsRegistryComponent(getAddrByID(comps, IsRegistryCompID)).set(id); | ||
| TypeComponent(getAddrByID(comps, TypeCompID)).set(id, type_); | ||
| NameComponent(getAddrByID(comps, NameCompID)).set(id, type_); | ||
| DescriptionComponent(getAddrByID(comps, DescriptionCompID)).set(id, description); | ||
| } |
There was a problem hiding this comment.
Add existence check to prevent overwriting existing registries.
The create function does not verify whether a registry with the given type_ already exists. Calling it twice with the same type will silently overwrite the existing entry, potentially losing associated flags or owner component mappings.
Consider adding a check:
function create(
IUintComp comps,
string memory type_,
string memory description
) internal returns (uint256 id) {
id = genID(type_);
+ require(!isInstance(comps, id), "ShapeRegistry: already exists");
LibEntityType.set(comps, id, "SHAPE_REGISTRY");
IsRegistryComponent(getAddrByID(comps, IsRegistryCompID)).set(id);
TypeComponent(getAddrByID(comps, TypeCompID)).set(id, type_);
NameComponent(getAddrByID(comps, NameCompID)).set(id, type_);
DescriptionComponent(getAddrByID(comps, DescriptionCompID)).set(id, description);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/contracts/src/libraries/LibShapeRegistry.sol around lines 28 to 39,
the create(...) function currently generates an id and proceeds to set registry
components without checking for an existing registry, which allows silent
overwrites; modify the function to compute id = genID(type_) then check whether
the registry already exists (e.g.,
require(!IsRegistryComponent(getAddrByID(comps, IsRegistryCompID)).get(id),
"Shape registry exists") or equivalent using the TypeComponent) and revert if it
does, before performing any set(...) calls so existing registries and their
associated flags/owners are preserved.
chlwys
force-pushed
the
ap.shape_registry_system
branch
from
58c3346 to
3bd12d0
Compare
1 participant
Shape registry: To store top level data for specific shapes. For example, if
KAMIcan be transferred internally, or ifINVENTORYcan be traded.EntityType) instead of raw componentIDsSummary by CodeRabbit