feat(sozo): add parameter to control abi format in manifest #3387
Conversation
|
Ohayo sensei! 👋 WalkthroughThis PR adds support for selecting how ABIs are written in generated manifests via a new CLI option. Users can choose between AllInOne format (global ABI map) or PerContract format (per-contract inline ABIs), with configuration read from migration settings and merged with CLI arguments. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as MigrateArgs<br/>(CLI Parser)
participant Config as MigrationConfig
participant Manifest
User->>CLI: Provide --manifest-abi-format
CLI->>CLI: Parse ManifestAbiFormatArg
CLI->>Config: Read per-migration config
Config-->>CLI: Return manifest_abi_format
CLI->>CLI: Merge CLI arg with config<br/>(CLI takes priority)
CLI->>CLI: Convert to ManifestAbiFormat
CLI->>Manifest: Call apply_abi_format(format)
alt AllInOne Format
Manifest->>Manifest: strip_inline_abis()
Manifest-->>User: Global abis map retained
else PerContract Format
Manifest->>Manifest: Clear global abis map
Manifest-->>User: Per-contract inline ABIs retained
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 0
🧹 Nitpick comments (2)
crates/dojo/world/src/config/migration_config.rs (1)
3-14: Ohayo, sensei! Clean enum definition with a minor simplification opportunity.The enum and default impl look good. Consider using the
#[default]derive attribute to reduce boilerplate:🔎 Suggested simplification
-#[derive(Debug, Clone, Deserialize)] +#[derive(Debug, Clone, Default, Deserialize)] #[serde(rename_all = "snake_case")] pub enum ManifestAbiFormat { + #[default] AllInOne, PerContract, } - -impl Default for ManifestAbiFormat { - fn default() -> Self { - Self::AllInOne - } -}bin/sozo/src/commands/migrate.rs (1)
43-46: Ohayo sensei! Consider adding help text for clarity.The option works, but users running
--helpwould benefit from knowing what each format does:🔎 Add descriptive help text
/// Select how ABIs are written in the generated manifest. - #[arg(long, value_enum, value_name = "FORMAT")] + #[arg(long, value_enum, value_name = "FORMAT", help = "Select how ABIs are written in the generated manifest: 'all_in_one' merges ABIs into a global map, 'per_contract' keeps ABIs inline per contract.")] pub manifest_abi_format: Option<ManifestAbiFormatArg>,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/sozo/src/commands/migrate.rs(5 hunks)crates/dojo/world/src/config/migration_config.rs(2 hunks)crates/dojo/world/src/diff/manifest.rs(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-05T04:30:12.852Z
Learnt from: glihm
Repo: dojoengine/dojo PR: 2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Applied to files:
crates/dojo/world/src/diff/manifest.rs
📚 Learning: 2025-10-30T22:20:34.112Z
Learnt from: glihm
Repo: dojoengine/dojo PR: 3370
File: crates/dojo/world/src/diff/manifest.rs:454-523
Timestamp: 2025-10-30T22:20:34.112Z
Learning: In Cairo ABIs, functions are typically inside interfaces whose names include the full Cairo path (e.g., `dojo::core::IWorldProvider`), making interface names inherently unique. When deduplicating ABI entries in Dojo's manifest by name (get_abi_name), collisions don't occur because interface names encode their paths, and Impl entries are intentionally filtered out while keeping interfaces.
Applied to files:
crates/dojo/world/src/diff/manifest.rs
⏰ 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: fmt
🔇 Additional comments (7)
crates/dojo/world/src/config/migration_config.rs (1)
26-27: LGTM!The optional field integrates cleanly with the existing config structure, sensei.
bin/sozo/src/commands/migrate.rs (2)
91-97: LGTM! Clean precedence logic.The priority chain (CLI → config → default) is correctly implemented, sensei. This is idiomatic Rust with proper use of
Optioncombinators.
140-154: LGTM! Proper separation between CLI and domain types.Having a distinct
ManifestAbiFormatArgfor CLI parsing keeps theclapdependency isolated from the core config module. TheFromconversion is straightforward, sensei.crates/dojo/world/src/diff/manifest.rs (4)
29-35: Ohayo sensei! Good serde configuration.The combination of
defaultfor deserialization andskip_serializing_if = "HashMap::is_empty"for serialization correctly handles both reading manifests with missingabisand writing manifests where the field should be omitted inPerContractmode.
53-55: LGTM! Consistent serde attributes across all ABI-bearing structs.All inline
abifields now use#[serde(default, skip_serializing_if = "Vec::is_empty")], which enables clean omission in theAllInOneformat while still supporting deserialization, sensei.Also applies to: 67-69, 88-90, 115-117, 144-146, 172-174
256-272: LGTM! Clean implementation of the format selection logic.The two methods work well together:
strip_inline_abisclears per-resource ABIs forAllInOnemodeapply_abi_formatdispatches based on the chosen formatThis correctly handles the starknet.js limitation mentioned in the PR objectives. Based on learnings, Cairo ABI interface names include full paths making them inherently unique, so the global
abismap deduplication is safe, sensei.
553-611: Ohayo! Solid test coverage, sensei.The test validates all three serialization scenarios:
- Default (before applying format) → both inline and global ABIs present
AllInOne→ inline ABIs stripped, globalabisretainedPerContract→ inline ABIs retained, globalabisclearedThis directly tests the serialization behavior which is the core functionality of this PR.
Due to some limitation in straknet.js and ABI parsing in Dojo.js, this update allows the user to define the ABI format in the manifest.
Currently the limitation is that, if two contracts have the same function name, but different parameters, the lookup in the ABI will only keep the first one found. Which leads to mismatch between the function to be called in certain cases.
This update introduces two level of configuration:
manifest_abi_formatkey under[migration]could be set toper_contractorall_in_one.manifest-abi-formatthat accepts the exact same valuesper_contractorall_in_one.By default, the
all_in_oneis used. So if the across the whole dojo project there is no duplicated function names, it will work fine. Otherwise, it is required for now to switch toper_contractABI.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.