fix(sozo): update Controller dependencies #2303
Conversation
WalkthroughOhayo, sensei! Recent updates refine the Rust project's structure with significant enhancements to dependency management, session handling, account representations, and import organization. Key modifications include transitioning to precise dependency revisions, improving session management logic for efficiency, and clarifying account type distinctions. Additionally, a new constant for the controller account contract class hash has been introduced, along with corresponding tests to ensure its correctness. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant SessionManager
User->>Controller: Request session
Controller->>SessionManager: Check existing session
alt Session exists
SessionManager->>Controller: Return existing session
else New session
Controller->>SessionManager: Create new session
SessionManager->>Controller: Return new session
end
Controller-->>User: Provide session
Possibly related PRs
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (1)
Additional comments not posted (1)
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
Outside diff range, codebase verification and nitpick comments (1)
crates/torii/graphql/src/tests/mod.rs (1)
22-23: Ohayo, sensei! TheCalltype is still in use.The
Calltype is used in theexecute_v1function calls within this module. Its removal from the imports will lead to errors. Please ensure the import statement forCallis retained.
- Lines using
Call:
execute_v1(vec![Call { ... }]).send()Analysis chain
Removal of
Calltype from imports.The
Calltype has been removed from the imports. Make sure that this change does not impact any existing functionality that relied onCall. Verify that the code no longer requires this type.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `Call` type is no longer needed in this module. # Test: Search for `Call` usage in this module. Expect: No usage of `Call`. rg --type rust -A 5 $'Call' crates/torii/graphql/src/tests/mod.rsLength of output: 760
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
bin/sozo/src/commands/options/account/type.rs (1)
5-5: Consolidate import statements for better clarity.The import statements could be grouped by their source or functionality to improve readability.
use starknet::accounts::{ single_owner, Account, ConnectedAccount, DeclarationV2, DeclarationV3, ExecutionEncoder, ExecutionV1, ExecutionV3, LegacyDeclaration, RawDeclarationV2, RawDeclarationV3, RawExecutionV1, RawExecutionV3, RawLegacyDeclaration, SingleOwnerAccount, }; use starknet::core::types::{ contract::legacy::LegacyContractClass, BlockId, Call, Felt, FlattenedSierraClass, }; use starknet::providers::Provider; use starknet::signers::{local_wallet, LocalWallet, SignerInteractivityContext};Also applies to: 10-10, 12-12
crates/katana/controller/src/lib.rs (1)
4-5: Remove unused import statements.The commented-out imports should be removed to keep the code clean.
-// use account_sdk::signers::webauthn::{DeviceSigner, WebauthnAccountSigner}; // use account_sdk::wasm_webauthn::CredentialID;Also applies to: 7-7
Cargo.toml (1)
227-230: Local paths foraccount_sdkandslotare missing.Ohayo, sensei! The local paths specified for
account_sdkandslotin theCargo.tomlfile do not exist in the expected locations. This might affect the build process, so please ensure these paths are correct or update them accordingly.
account_sdkpath:../../cartridge/controller/packages/account_sdkslotpath:../../cartridge/slot/slotAnalysis chain
Local path usage for
account_sdkandslot.Using local paths can enhance build performance but ensure that these local versions are up-to-date and compatible with the rest of the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the local path usage for `account_sdk` and `slot`. # Test: Check the existence of the local paths. if [[ -d "../../cartridge/controller/packages/account_sdk" && -d "../../cartridge/slot/slot" ]]; then echo "Local paths exist." else echo "Local paths are missing." fiLength of output: 117
bin/sozo/src/commands/options/account/controller.rs (1)
39-39: Ohayo, sensei! UsingArcfor shared ownership is a smart move.Switching to
Arc<P>resolves theClonetrait bound issue, enabling shared ownership of the provider. This change enhances flexibility and adheres to Rust's ownership model.Consider evaluating the performance implications of using
Arcif the provider is accessed frequently in a multi-threaded context.
glihm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
examples/spawn-and-move/dojo_release.toml (1)
22-23: Acknowledged: Improved formatting inskip_contracts.The indentation changes improve the readability of the
skip_contractslist. Nice touch!bin/sozo/src/commands/options/account/controller.rs (1)
32-35: Clarify the use ofArcin comments.The comments explain the reason for using
Arcdue toClonebounds. Ensure that this explanation is clear to maintainers and future developers.- // Why the Arc? becaues the Controller account implementation over on `account_sdk` crate is + // Why the Arc? Because the Controller account implementation in the `account_sdk` crate is
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
crates/katana/executor/tests/fixtures/transaction.rs (2)
9-12: Update import statements.The import of
BroadcastedInvokeTransactionV1is necessary for the new transaction structure. Ensure thatBroadcastedInvokeTransactionis still required; otherwise, consider removing it to keep imports clean.
49-49: Clarify theis_queryflag usage.The
is_queryflag is set tofalse, indicating this transaction is not a query. Consider documenting its purpose for future maintainers.crates/katana/controller/src/lib.rs (1)
4-5: Evaluate the commented-out imports.The imports for
DeviceSignerandWebauthnAccountSignerare commented out. Determine if they are still needed or if they can be removed to clean up the code.crates/torii/graphql/src/tests/mod.rs (1)
21-22: Ohayo, sensei! TheCalltype is still in use.The
Calltype is still being used incrates/torii/graphql/src/tests/mod.rs, and its removal from the imports will likely cause compilation errors. Please ensure that the import is retained or that the usage ofCallis refactored if it is no longer needed.
crates/torii/graphql/src/tests/mod.rs: Lines showing usage ofCall.Analysis chain
Ohayo, sensei! Verify the impact of removing
Call.The
Calltype has been removed from the imports. Ensure that any functionality that previously depended onCallis not affected by this change.Run the following script to check for any remaining usages of
Call:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `Call` is still used in the codebase. # Test: Search for occurrences of `Call`. Expect: No occurrences. rg --type rust 'Call'Length of output: 25055
kariy
force-pushed
the
sozo/controller
branch
from
9d41ea4 to
b49b5f2
Compare
kariy
force-pushed
the
sozo/controller
branch
from
3bafdaf to
13ae133
Compare
kariy
force-pushed
the
sozo/controller
branch
from
13ae133 to
a516b5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
Cargo.toml (2)
120-120: Theasync-traitdependency update is a minor version bump.Upgrading from version
0.1.77to0.1.82should not introduce any breaking changes, but it's always a good idea to review the changelog and test thoroughly.
241-241: Thestarknet-cryptodependency has a minor version downgrade.Downgrading from version
0.7.0to0.7.1should not introduce any issues, but please double-check the changelog to ensure compatibility.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (23)
- Cargo.toml (6 hunks)
- bin/sozo/Cargo.toml (1 hunks)
- bin/sozo/src/commands/options/account/controller.rs (10 hunks)
- bin/sozo/src/commands/options/account/mod.rs (2 hunks)
- bin/sozo/src/commands/options/account/type.rs (4 hunks)
- crates/dojo-world/src/contracts/abi/model.rs (1 hunks)
- crates/dojo-world/src/migration/mod.rs (1 hunks)
- crates/katana/controller/Cargo.toml (1 hunks)
- crates/katana/controller/src/lib.rs (5 hunks)
- crates/katana/controller/src/webauthn.rs (1 hunks)
- crates/katana/core/src/service/messaging/starknet.rs (1 hunks)
- crates/katana/executor/Cargo.toml (1 hunks)
- crates/katana/executor/tests/fixtures/transaction.rs (3 hunks)
- crates/katana/rpc/rpc/tests/common/mod.rs (1 hunks)
- crates/katana/rpc/rpc/tests/starknet.rs (2 hunks)
- crates/katana/rpc/rpc/tests/torii.rs (1 hunks)
- crates/saya/core/src/dojo_os/mod.rs (1 hunks)
- crates/saya/core/src/verifier/starknet.rs (1 hunks)
- crates/sozo/ops/src/execute.rs (1 hunks)
- crates/sozo/ops/src/migration/mod.rs (1 hunks)
- crates/torii/core/src/sql_test.rs (1 hunks)
- crates/torii/graphql/src/tests/mod.rs (1 hunks)
- crates/torii/grpc/src/server/tests/entities_test.rs (1 hunks)
Files skipped from review due to trivial changes (8)
- crates/katana/core/src/service/messaging/starknet.rs
- crates/katana/executor/Cargo.toml
- crates/katana/rpc/rpc/tests/common/mod.rs
- crates/saya/core/src/dojo_os/mod.rs
- crates/saya/core/src/verifier/starknet.rs
- crates/sozo/ops/src/migration/mod.rs
- crates/torii/core/src/sql_test.rs
- crates/torii/graphql/src/tests/mod.rs
Files skipped from review as they are similar to previous changes (12)
- bin/sozo/Cargo.toml
- bin/sozo/src/commands/options/account/controller.rs
- bin/sozo/src/commands/options/account/mod.rs
- bin/sozo/src/commands/options/account/type.rs
- crates/dojo-world/src/contracts/abi/model.rs
- crates/dojo-world/src/migration/mod.rs
- crates/katana/controller/src/webauthn.rs
- crates/katana/executor/tests/fixtures/transaction.rs
- crates/katana/rpc/rpc/tests/starknet.rs
- crates/katana/rpc/rpc/tests/torii.rs
- crates/sozo/ops/src/execute.rs
- crates/torii/grpc/src/server/tests/entities_test.rs
Additional comments not posted (13)
crates/katana/controller/Cargo.toml (3)
12-12: Ohayo sensei! Theasync-traitdependency is a great addition.The
async-traitcrate is a popular and stable choice for enabling async methods in traits. It will enhance the module's ability to handle asynchronous programming. Nicely done!
21-23: Verify the reason for using the Git dependency forwebauthn-rs-proto.I see that you've added the
webauthn-rs-protodependency from a specific Git commit to ensure compatibility withaccount_sdk. While this ensures compatibility, it may make it harder to update the dependency in the future.Is there a specific reason for using the Git dependency instead of a version from crates.io? If not, consider using a crates.io version for better stability and easier updates.
25-25: Theassert_matchesdependency is a solid choice for testing, sensei!Adding the
assert_matchescrate under[dev-dependencies]is perfect. It will make the tests more expressive and readable. Good thinking!Cargo.toml (5)
64-64: Ohayo, sensei! Thecainomedependency update looks good.The change from using a version tag to a specific commit hash provides more control over the exact version being used, ensuring compatibility and stability.
161-161: Ohayo! The addition of thehashlinkdependency suggests new functionality.Please ensure that the new functionality introduced by this dependency is properly tested and documented.
230-230: Ohayo! Theslotdependency update simplifies the dependency tree.By using a version tag instead of a commit hash and re-exporting
account_sdk, the dependency management becomes cleaner and more maintainable.
240-240: Thestarknetdependency update to a specific commit hash provides more control.However, please ensure that the referenced commit (
2ddc694) is stable and thoroughly tested.Also applies to: 249-249
242-246: Ohayo, sensei! Thestarknet-types-coredependency update is necessary for compatibility.The strict version requirement of
~0.1.4ensures that the project uses a version that includes the necessary changes from the referenced pull request. Updating the dependency to a specific commit hash (f98f048) further ensures compatibility.Also applies to: 250-250
crates/katana/controller/src/lib.rs (5)
129-192: Ohayo sensei! Theget_contract_storagefunction looks great!The code segment follows a clear logic to compute the
guidusing theWebauthnSignerandSlotBackend. The use of these structures improves modularity and testability. The unimplemented functions inSlotBackendare safely handled by returning errors since they are not called during theguidcomputation. The storage key and value for the owner's credentials are correctly computed.
Line range hint
197-248: Ohayo sensei! The test cases are excellent!The test cases cover the important aspects of the
add_controller_accountandget_contract_storagefunctions using realistic test data. The assertions ensure that the functions behave as expected by checking the genesis allocations and storage values. These test cases improve the overall reliability and maintainability of the codebase. Great job!
162-179: Ohayo sensei! TheWebauthnBackendtrait implementation forSlotBackendis safe and sound!The
get_assertionandcreate_credentialfunctions are not implemented and return errors, as indicated by theSAFETYcomment. This is safe since these functions are not called during theguidcomputation. Returning errors avoids unnecessary complexity. The code segment is well-structured and follows the trait implementation guidelines.
151-155: Ohayo sensei! TheOriginProvidertrait implementation forSlotBackendis perfect!The
originfunction returns theWEBAUTHN_ORIGINconstant, which is the expected behavior. The code segment is well-structured and follows the trait implementation guidelines. Keep up the great work!
5-14: Ohayo sensei! The new imports are spot-on!The added imports for
async_trait,CoseKey,WebauthnBackend,WebauthnSigner, andOriginProviderare necessary for the WebAuthn-related functionality introduced in the code changes. The imports are correctly specified and follow the Rust naming conventions. The organization and grouping of the imports improve the readability and maintainability of the code. Excellent work!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2303 +/- ##
==========================================
- Coverage 67.92% 67.91% -0.01%
==========================================
Files 364 364
Lines 47832 47910 +78
==========================================
+ Hits 32490 32539 +49
- Misses 15342 15371 +29 ☔ View full report in Codecov by Sentry. |
includes bunch of dependencies version bump to avoid versioning issues
Summary by CodeRabbit
New Features
SozoAccountenum.libp2pdependencies to ensure consistency and incorporate recent improvements.Bug Fixes
Documentation
Chores