feat(dojo-world): change manifest format including members for events and models#3370
feat(dojo-world): change manifest format including members for events and models#3370glihm merged 3 commits intodojoengine:mainfrom
Conversation
WalkthroughOhayo, sensei! The PR centralizes ABI entries into a Manifest-level Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-11-28T23:35:04.367ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/dojo/world/src/contracts/contract_info.rs(1 hunks)crates/dojo/world/src/diff/manifest.rs(14 hunks)crates/dojo/world/src/local/artifact_to_local.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-05T04:30:12.852Z
Learnt from: glihm
PR: dojoengine/dojo#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/contracts/contract_info.rscrates/dojo/world/src/diff/manifest.rs
🧬 Code graph analysis (3)
crates/dojo/world/src/local/artifact_to_local.rs (1)
crates/dojo/world/src/local/resource.rs (1)
abi(175-187)
crates/dojo/world/src/contracts/contract_info.rs (3)
crates/dojo/world/src/local/artifact_to_local.rs (1)
abi(98-101)crates/dojo/world/src/local/resource.rs (1)
abi(175-187)crates/dojo/world/src/diff/manifest.rs (1)
new(198-252)
crates/dojo/world/src/diff/manifest.rs (2)
crates/dojo/world/src/local/resource.rs (3)
name(130-142)name(271-276)abi(175-187)crates/dojo/world/src/local/artifact_to_local.rs (1)
abi(98-101)
⏰ 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
| /// Gets the name of the ABI entry. | ||
| fn get_abi_name(abi_entry: &AbiEntry) -> String { | ||
| match abi_entry { | ||
| AbiEntry::Function(function) => function.name.clone(), | ||
| AbiEntry::Event(event) => match event { | ||
| AbiEvent::Typed(TypedAbiEvent::Struct(s)) => s.name.clone(), | ||
| AbiEvent::Typed(TypedAbiEvent::Enum(e)) => e.name.clone(), | ||
| AbiEvent::Untyped(UntypedAbiEvent { name, .. }) => name.clone(), | ||
| }, | ||
| AbiEntry::Struct(struct_) => struct_.name.clone(), | ||
| AbiEntry::Enum(enum_) => enum_.name.clone(), | ||
| AbiEntry::Constructor(constructor) => constructor.name.clone(), | ||
| AbiEntry::Impl(impl_) => impl_.name.clone(), | ||
| AbiEntry::Interface(interface) => interface.name.clone(), | ||
| AbiEntry::L1Handler(l1_handler) => l1_handler.name.clone(), | ||
| } | ||
| } | ||
|
|
||
| /// Serializes the ABI entries into a vector sorted alphabetically by name. | ||
| /// This ensures compatibility with any tool expecting a Cairo contract ABI. | ||
| fn serialize_abis_hashmap<S>( | ||
| value: &HashMap<String, AbiEntry>, | ||
| serializer: S, | ||
| ) -> std::result::Result<S::Ok, S::Error> | ||
| where | ||
| S: serde::Serializer, | ||
| { | ||
| let mut seq = serializer.serialize_seq(Some(value.len()))?; | ||
|
|
||
| // Sort alphabetically by name. | ||
| let mut sorted_entries = value.values().collect::<Vec<_>>(); | ||
| sorted_entries.sort_by_key(|e| get_abi_name(e)); | ||
|
|
||
| for abi_entry in sorted_entries { | ||
| seq.serialize_element(abi_entry)?; | ||
| } | ||
|
|
||
| seq.end() | ||
| } | ||
|
|
||
| /// Deserializes the ABI entries from a vector into a hashmap. | ||
| fn deserialize_abis_hashmap<'de, D>( | ||
| deserializer: D, | ||
| ) -> std::result::Result<HashMap<String, AbiEntry>, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let entries = Vec::<AbiEntry>::deserialize(deserializer)?; | ||
|
|
||
| let mut map = HashMap::new(); | ||
|
|
||
| for abi_entry in entries { | ||
| map.insert(get_abi_name(&abi_entry), abi_entry); | ||
| } | ||
|
|
||
| Ok(map) | ||
| } | ||
|
|
||
| /// Adds the ABI entries to the manifest, deduplicating them by name. | ||
| fn add_abi_entries(abis: &mut HashMap<String, AbiEntry>, abi: Vec<AbiEntry>) { | ||
| for abi_entry in abi { | ||
| // We can strip out `impl` type entries, since they are not meaningful for the manifest. | ||
| // Keeping the interface is enough. | ||
| if matches!(abi_entry, AbiEntry::Impl(_)) { | ||
| continue; | ||
| } | ||
|
|
||
| let entry_name = get_abi_name(&abi_entry); | ||
| abis.insert(entry_name, abi_entry); | ||
| } |
There was a problem hiding this comment.
Ohayo sensei: ABI dedup is clobbering shared function names
Keying the global abis map by get_abi_name means every AbiEntry::Function collapses down to its bare name (e.g. all constructor, upgrade, ensure_abi, etc.). As soon as two different contracts expose a function with the same name, the later add_abi_entries call overwrites the earlier one, so we silently drop ABI entries. Once serialized/deserialized, those contracts have incomplete ABIs and we can’t faithfully reconstruct selectors or entrypoints. Please dedupe with a collision-free key (e.g. include the entry kind plus full path/signature, or store multiple entries per name) so that identically named functions from different contracts survive the merge.
🤖 Prompt for AI Agents
In crates/dojo/world/src/diff/manifest.rs around lines 454 to 523, the global
abis HashMap keyed by get_abi_name causes different contracts' ABI entries with
the same bare name to clobber each other; change the dedup key to be
collision-free. Replace the current String key strategy with either (A) a
composed unique key (e.g. include entry kind and full signature/path:
format!("{}|{}|{}", kind, contract_path_or_type, signature)) by implementing a
new get_abi_key(abi_entry: &AbiEntry) -> String and use it everywhere
get_abi_name is used for map keys, or (B) change the map type to HashMap<String,
Vec<AbiEntry>> and append entries to the vec for a given name (so identical
names from different contracts are preserved) and update serialize_abis_hashmap,
deserialize_abis_hashmap and add_abi_entries accordingly to handle the new
key/value shape; ensure Impl entries are still skipped and adjust
sorting/serialization to produce stable, deterministic output.
There was a problem hiding this comment.
It will not happen, since most of the time functions are inside interfaces, which are differently named inherently due to the cairo path of the interface itself.
There was a problem hiding this comment.
@glihm Ohayo sensei! Thank you for the clarification. That makes perfect sense—since Cairo interfaces include the full path in their name (e.g., dojo::core::IWorldProvider rather than just bare function names), the keying strategy naturally avoids collisions. Functions accessed through different interfaces will have unique names in the abis map because the interface name itself encodes the path.
I appreciate you taking the time to explain this Cairo-specific ABI structure detail! 🙏
🐰✨
(noting this Cairo ABI wisdom for future reviews)
✏️ Learnings added
Learnt from: glihm
Repo: dojoengine/dojo PR: 3370
File: crates/dojo/world/src/diff/manifest.rs:454-523
Timestamp: 2025-10-30T22:20:34.102Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
1 participant
This manifest change is backward compatible, and adds correctly the members to events and models.
The format is not having one single
abisentry instead of repeatedabikeys that were impacting the readability of the manifest.Summary by CodeRabbit