fix(dojo-lang): ensure the dns returns the actual ClassHash.
#2935
Conversation
WalkthroughOhayo, sensei! The pull request introduces a new test function Changes
Sequence DiagramsequenceDiagram
participant Test as DNS Test
participant World as World Storage
participant Syscall as Starknet Syscall
Test->>World: Call dns("bar")
World->>Syscall: get_class_hash_at_syscall(contract_address)
Syscall-->>World: Return class_hash
World-->>Test: Return (contract_address, class_hash)
The sequence diagram illustrates the new flow of retrieving a class hash through a syscall when performing a DNS lookup in the world storage implementation. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (1)
crates/dojo/core-cairo-test/src/tests/world/world.cairo (1)
314-331: Consider adding more test cases, sensei!To make the test suite more robust, consider adding:
- Test for non-existent contract names
- Test for multiple contracts in the same namespace
- Test for contracts after upgrade
#[test] pub fn dns_valid_class_hash() { let namespace_def = NamespaceDef { namespace: "dojo", resources: [ TestResource::Model(m_Foo::TEST_CLASS_HASH), TestResource::Contract(bar::TEST_CLASS_HASH), + TestResource::Contract(test_contract::TEST_CLASS_HASH), ] .span(), }; let mut world = spawn_test_world([namespace_def].span()); world.sync_perms_and_inits([].span()); let (_, class_hash) = world.dns(@"bar").unwrap(); assert!(class_hash == bar::TEST_CLASS_HASH.try_into().unwrap()); + + // Test non-existent contract + assert!(world.dns(@"nonexistent").is_none()); + + // Test another contract in same namespace + let (_, class_hash2) = world.dns(@"test_contract").unwrap(); + assert!(class_hash2 == test_contract::TEST_CLASS_HASH.try_into().unwrap()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (2)
crates/dojo/core-cairo-test/src/tests/world/world.cairo(2 hunks)crates/dojo/core/src/world/storage.cairo(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
crates/dojo/core/src/world/storage.cairo (1)
45-50: Ohayo sensei! The DNS implementation now correctly retrieves the actual ClassHash.The change to use
get_class_hash_at_syscallinstead of the stored class hash is a robust solution that ensures we always get the current class hash, even if the contract was upgraded.crates/dojo/core-cairo-test/src/tests/world/world.cairo (1)
314-331: LGTM! The test validates the DNS fix.The test effectively verifies that the DNS returns the correct class hash for a registered contract.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2935 +/- ##
==========================================
+ Coverage 55.80% 55.82% +0.01%
==========================================
Files 444 444
Lines 57329 57329
==========================================
+ Hits 31994 32002 +8
+ Misses 25335 25327 -8 ☔ View full report in Codecov by Sentry. |
Since this class hash is not used often, it wasn't detected before.
Added a test to avoid regression.
Previously it returned the namespace hash.
Summary by CodeRabbit
Tests
dojonamespaceImprovements