fix(sozo): return error if inspect doesn't find resource #2920
Conversation
…if resource not found
WalkthroughOhayo, sensei! This pull request introduces enhancements to the Dojo project's CI workflow, modifies the Changes
Sequence DiagramsequenceDiagram
participant CI as GitHub Actions
participant Sozo as Sozo Binary
participant Build as Build Process
participant Test as Test Execution
CI->>Sozo: Run build command
Sozo->>Build: Build external contracts
Build-->>Sozo: Build complete
Sozo->>Sozo: Inspect resources
Sozo->>Test: Execute tests
Test-->>CI: Report results
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (2)
🪧 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: 1
🧹 Nitpick comments (2)
examples/spawn-and-move/src/actions.cairo (1)
323-355: Ohayo sensei! Let's make this test more robust!While the test setup is well-structured, it could benefit from additional verifications:
- Add assertions to verify the state after entering the dungeon
- Consider testing error scenarios (e.g., invalid dungeon address)
Here's a suggested enhancement:
actions_system.enter_dungeon(dungeon_addr); + +// Verify Flatbow was created correctly +let flatbow: armory::Flatbow = world.read_model(1); +assert(flatbow.atk_speek == 2, 'wrong flatbow attack speed'); +assert(flatbow.range == 1, 'wrong flatbow range'); + +// Verify RiverSkale was created correctly +let river_skale: bestiary::RiverSkale = world.read_model(1); +assert(river_skale.health == 5, 'wrong river_skale health'); +assert(river_skale.armor == 3, 'wrong river_skale armor'); +assert(river_skale.attack == 2, 'wrong river_skale attack'); + +// Add error case +let invalid_dungeon = starknet::contract_address_const::<0x0>(); +should_panic( + || { actions_system.enter_dungeon(invalid_dungeon); }, + 'should fail with invalid dungeon' +);bin/sozo/src/utils.rs (1)
120-126: Ohayo sensei! Nice improvement to error handling!The change from error to warning level is appropriate here since the code gracefully falls back to local inspection. The message is also more informative about the implications.
Consider adding a trace log with the actual error from the health check for debugging purposes:
if (provider_utils::health_check_provider(provider.clone()).await).is_err() { + trace!(target: LOG_TARGET, "Provider health check failed with error: {:?}", err); warn!(target: LOG_TARGET, "Provider health check failed during sozo inspect, inspecting locally and all resources will appeared as `Created`. Remote resources will not be fetched.");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
bin/sozo/tests/test_data/invalid_cairo_version/Scarb.lockis excluded by!**/*.lockcrates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/ci.yml(2 hunks)bin/sozo/src/commands/inspect.rs(4 hunks)bin/sozo/src/utils.rs(2 hunks)examples/spawn-and-move/Scarb.toml(1 hunks)examples/spawn-and-move/src/actions.cairo(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (4)
examples/spawn-and-move/src/actions.cairo (1)
231-231: Ohayo! Import looks good, sensei!The dungeon module import is properly placed and follows the crate's module structure.
bin/sozo/src/commands/inspect.rs (2)
169-169: Ohayo sensei! Great improvement to error handling!Converting the function to return
Result<()>and propagating the error is a much better approach than just printing a message. This allows callers to handle the "resource not found" case appropriately.Also applies to: 178-178
245-245: Clean implementation of the success case!The addition of
Ok(())properly completes the Result return type implementation..github/workflows/ci.yml (1)
153-155: Ohayo sensei! CI changes look solid!The addition of
sozo buildandsozo inspectcommands for the new namespaces ensures proper testing of both the external contracts and the enhanced inspect functionality.
| @@ -10,7 +10,7 @@ edition = "2024_07" | |||
| sierra-replace-ids = true | |||
|
|
|||
| [[target.starknet-contract]] | |||
| build-external-contracts = [ "dojo::world::world_contract::world" ] | |||
| build-external-contracts = [ "dojo::world::world_contract::world", "armory::m_Flatbow", "bestiary::m_RiverSkale" ] | |||
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! We've got some missing contracts!
The external contracts armory::m_Flatbow and bestiary::m_RiverSkale referenced in build-external-contracts don't exist in the repository. Please ensure:
- The contract files are implemented and added to the repository
- The module names are correct
- Any required dependencies are properly set up
🔗 Analysis chain
Ohayo sensei! External contracts addition looks good!
The new external contracts armory::m_Flatbow and bestiary::m_RiverSkale are properly added and align with the declared dependencies.
Let's verify that these modules exist in the referenced paths:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the external contract modules exist
fd -t f "m_Flatbow.cairo$" "../game-lib/armory"
fd -t f "m_RiverSkale.cairo$" "../game-lib/bestiary"
Length of output: 314
Script:
#!/bin/bash
# Search for the contract files in the repository
fd -t f "m_Flatbow.cairo$"
fd -t f "m_RiverSkale.cairo$"
# Also check if these are referenced in any other configuration files
rg "m_Flatbow|m_RiverSkale" -t toml
Length of output: 248
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2920 +/- ##
=======================================
Coverage 55.90% 55.91%
=======================================
Files 443 443
Lines 57190 57192 +2
=======================================
+ Hits 31973 31978 +5
+ Misses 25217 25214 -3 ☔ View full report in Codecov by Sentry. |
If
sozo inspectdoesn't find a resource, it returns an error. Also, if the provider is not healthy,sozo inspectchecks only the local resources.This is a good way to check if some external contracts have been correctly added to the
Scarb.toml.Summary by CodeRabbit
Release Notes
New Features
CI/CD Updates
Technical Improvements