Update nonce channels to use masks #891
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (14)
📒 Files selected for processing (2)
🧰 Additional context used🔇 Additional comments (8)
🪧 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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (13)
Scarb.tomlis excluded by none and included by nonepackages/account-wasm/src/account.rsis excluded by none and included by nonepackages/account-wasm/src/session.rsis excluded by none and included by nonepackages/account_sdk/artifacts/controller.latest.compiled_contract_class.jsonis excluded by none and included by nonepackages/account_sdk/artifacts/controller.latest.contract_class.jsonis excluded by none and included by nonepackages/account_sdk/cmd/bench.rsis excluded by none and included by nonepackages/account_sdk/src/account/outside_execution.rsis excluded by none and included by nonepackages/account_sdk/src/artifacts.rsis excluded by none and included by nonepackages/account_sdk/src/controller.rsis excluded by none and included by nonepackages/account_sdk/src/execute_from_outside.rsis excluded by none and included by nonepackages/account_sdk/src/execute_from_outside_test.rsis excluded by none and included by nonepackages/account_sdk/src/tests/runners/test_data/session_execute_outside.jsonis excluded by none and included by nonepackages/account_sdk/src/tests/runners/test_data/webauthn_execute_outside.jsonis excluded by none and included by none
📒 Files selected for processing (1)
- packages/account_sdk/src/abigen/controller.rs (8 hunks)
🧰 Additional context used
🔇 Additional comments (8)
packages/account_sdk/src/abigen/controller.rs (8)
242-242: Update serialization size calculation fornonceThe serialization size calculation now correctly uses the new type
<(starknet::core::types::Felt, u128)>. This ensures proper serialization of the updatednoncefield.
253-255: Ensure correct serialization of the updatednoncefieldThe serialization logic for
noncehas been updated to use<(starknet::core::types::Felt, u128)>::cairo_serialize(&__rust.nonce). This change correctly handles the new tuple structure of thenoncefield.
268-269: Update deserialization logic for the newnoncetypeThe deserialization process now uses the updated type
<(starknet::core::types::Felt, u128)>to correctly deserialize thenoncefield.
2260-2260: Update return type ofget_outside_execution_v3_channel_noncetou128The return type of
get_outside_execution_v3_channel_noncehas been updated fromFelttou128. This reflects the change in thenoncetype and ensures consistent handling of the nonce as au128value.
2365-2371: Updateis_valid_outside_execution_v3_nonceto accept the newnoncetypeThe method
is_valid_outside_execution_v3_noncenow accepts anonceparameter of type&(starknet::core::types::Felt, u128). The serialization of thenonceparameter has been updated accordingly.
2909-2909: Update return type ofget_outside_execution_v3_channel_nonceinControllerReadertou128The return type has been updated to
u128to match the changes in thenoncetype.
3014-3020: Updateis_valid_outside_execution_v3_nonceinControllerReaderto accept the newnoncetypeThe method now accepts the
nonceparameter as a&(starknet::core::types::Felt, u128), and the serialization logic has been updated to reflect this change.
Line range hint
230-269: Verify correct serialization and deserialization ofu128innonceThe
noncefield now includes au128type. Ensure that thecairo_serializeandcairo_deserializeimplementations correctly handle theu128value, considering any endianness or size considerations that may affect serialization between Rust and Cairo types.To automate the verification, you can run the following script:
| @@ -227,7 +227,7 @@ impl cainome::cairo_serde::CairoSerde for ExternalOwnerRemoved { | |||
| #[derive(Clone, serde::Serialize, serde::Deserialize, PartialEq, Debug)] | |||
| pub struct OutsideExecution { | |||
| pub caller: cainome::cairo_serde::ContractAddress, | |||
| pub nonce: (starknet::core::types::Felt, starknet::core::types::Felt), | |||
| pub nonce: (starknet::core::types::Felt, u128), | |||
There was a problem hiding this comment.
Consider the impact of changing nonce type on backward compatibility
Changing the nonce field from (Felt, Felt) to (Felt, u128) may affect clients or systems that rely on the previous structure. Ensure that this change is compatible with existing data and consider providing migration paths or versioning to handle this update smoothly.
💡 Codebase verification
Nonce Field Usage Requires Updates to Handle Tuple Structure
- In
packages/account_sdk/src/controller.rs:- Line 176:
.nonce(Felt::from(u64::MAX))should be updated to assign both elements of the tuple. - Line 237:
.nonce(nonce)should ensurenonceis a tuple. - Lines 245, 278, 294, 351: Operations like
self.nonce += Felt::ONE;and assignments need to handle both elements of the tuple.
- Line 176:
🔗 Analysis chain
Update the nonce field type to reflect the new tuple structure
The nonce field in the OutsideExecution struct has been updated to a tuple of (starknet::core::types::Felt, u128). Ensure that all references and usages of this field throughout the codebase correctly handle this new type.
To verify that all instances have been updated correctly, you can run the following script:
Run additional verification scripts to locate all nonce field accesses in OutsideExecution.
Run updated verification scripts targeting the correct directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all usages of 'OutsideExecution' and verify 'nonce' field access.
# Find all Rust files that contain 'OutsideExecution'.
fd --extension rs 'OutsideExecution'
# Search for 'nonce' field access in 'OutsideExecution' usages.
rg --type rust 'OutsideExecution.*\.nonce'
Length of output: 81
Script:
#!/bin/bash
# Description: Use ast-grep to find all accesses to the 'nonce' field in 'OutsideExecution' struct.
ast-grep --lang rust --pattern 'OutsideExecution { $$$, nonce: $_, $$$ }' src/**/*.rs
ast-grep --lang rust --pattern '$_ = OutsideExecution.nonce'
Length of output: 211
Script:
#!/bin/bash
# Description: Use ast-grep to find all accesses to the 'nonce' field in 'OutsideExecution' struct within packages/account_sdk.
# Search for struct definitions with the updated 'nonce' field.
ast-grep --lang rust --pattern 'OutsideExecution { $$$, nonce: $_, $$$ }' packages/account_sdk/**/*.rs
# Search for all accesses to the 'nonce' field in 'OutsideExecution'.
ast-grep --lang rust --pattern '$_.nonce' packages/account_sdk/**/*.rs
Length of output: 3706
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
packages/account_sdk/src/abigen/erc_20.rs (7)
Line range hint
145-153: Ensure Consistent Method Naming ConventionThe methods
balanceOfandbalance_ofare both defined inErc20andErc20Reader. Having duplicate methods with different naming conventions can cause confusion. Rust naming conventions prefersnake_casefor function and method names.Consider consolidating to a single method using
snake_case:- pub fn balanceOf(&self, account: &cainome::cairo_serde::ContractAddress) -> ... + pub fn balance_of(&self, account: &cainome::cairo_serde::ContractAddress) -> ...And update any references accordingly.
Also applies to: 246-254
Line range hint
160-168: Unify Method Naming fortotalSupplyBoth
totalSupplyandtotal_supplymethods are implemented. This duplication with differing naming styles can lead to errors. Stick tosnake_caseto align with Rust conventions.Update the method to use
snake_caseconsistently:- pub fn totalSupply(&self) -> ... + pub fn total_supply(&self) -> ...And adjust all calls to this method.
Also applies to: 260-268
Line range hint
181-192: Standardize Method Names for Transfer FunctionsMethods
transferFromandtransfer_fromare both present. Consistent naming enhances readability and prevents mistakes. Prefersnake_casein method names.Modify the method name to follow
snake_case:- pub fn transferFrom(&self, ...) -> ... + pub fn transfer_from(&self, ...) -> ...Ensure all invocations are updated to match the new name.
Also applies to: 281-292
Line range hint
195-206: Consistent Naming for Ownership Transfer MethodsThe methods
transferOwnershipandtransfer_ownershipare both defined. To maintain clarity, use a single naming style.Adopt
snake_casefor method names:- pub fn transferOwnership(&self, newOwner: &cainome::cairo_serde::ContractAddress) -> ... + pub fn transfer_ownership(&self, new_owner: &cainome::cairo_serde::ContractAddress) -> ...Update parameter names and usages accordingly.
Also applies to: 295-306
Line range hint
225-232: Handle Potential Enum Deserialization Errors GracefullyIn the
try_fromimplementation forERC20ComponentEvent, the panic messages useunwrap_or_elsewithpanic!, which can cause the entire program to exit on failure.Consider handling errors without panicking, returning an appropriate
Result::Errinstead:- .unwrap_or_else(|_| panic!("Invalid selector for {}", "Transfer")) + .map_err(|_| format!("Invalid selector for {}", "Transfer"))?Apply similar changes to other event parsing sections to improve robustness.
Line range hint
320-330: Consider Using Constants for SelectorsMagic strings like
"transfer"and"approve"are used directly in selectors. This can lead to typos and makes updates harder.Define constants for these selectors to improve maintainability:
const TRANSFER_SELECTOR: &str = "transfer"; const APPROVE_SELECTOR: &str = "approve";Use these constants in your code:
- selector: starknet::macros::selector!("transfer"), + selector: starknet::macros::selector!(TRANSFER_SELECTOR),
Line range hint
145-306: Remove Unused Duplicate MethodsHaving methods like
balanceOfandbalance_ofmay not be necessary if they perform the same functionality. This applies to other similarly duplicated methods.Retain only one version of each method to reduce redundancy and potential confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (14)
Cargo.tomlis excluded by none and included by noneScarb.tomlis excluded by none and included by nonepackages/account-wasm/src/account.rsis excluded by none and included by nonepackages/account-wasm/src/session.rsis excluded by none and included by nonepackages/account_sdk/artifacts/controller.latest.compiled_contract_class.jsonis excluded by none and included by nonepackages/account_sdk/artifacts/controller.latest.contract_class.jsonis excluded by none and included by nonepackages/account_sdk/cmd/bench.rsis excluded by none and included by nonepackages/account_sdk/src/account/outside_execution.rsis excluded by none and included by nonepackages/account_sdk/src/artifacts.rsis excluded by none and included by nonepackages/account_sdk/src/controller.rsis excluded by none and included by nonepackages/account_sdk/src/execute_from_outside.rsis excluded by none and included by nonepackages/account_sdk/src/execute_from_outside_test.rsis excluded by none and included by nonepackages/account_sdk/src/tests/runners/test_data/session_execute_outside.jsonis excluded by none and included by nonepackages/account_sdk/src/tests/runners/test_data/webauthn_execute_outside.jsonis excluded by none and included by none
📒 Files selected for processing (2)
- packages/account_sdk/src/abigen/controller.rs (10 hunks)
- packages/account_sdk/src/abigen/erc_20.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/account_sdk/src/abigen/erc_20.rs (1)
5-7:⚠️ Potential issueDuplicate Allow Attributes
The
#![allow(clippy::all)]and#![allow(warnings)]attributes are specified twice at the top of the file.Ensure that attributes are not duplicated to keep the code clean.
| #![allow(clippy::all)] | ||
| #![allow(warnings)] | ||
|
|
There was a problem hiding this comment.
Avoid Suppressing All Clippy Lints and Warnings
Adding #![allow(clippy::all)] and #![allow(warnings)] suppresses all Clippy lints and compiler warnings for the entire module. This can hide potential issues and decrease code quality. It's better to address warnings individually or allow specific lints when necessary.
Consider removing these directives or refining them to target specific warnings:
-#![allow(clippy::all)]
-#![allow(warnings)]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #![allow(clippy::all)] | |
| #![allow(warnings)] |
| pub nonce: (starknet::core::types::Felt, starknet::core::types::Felt), | ||
| #[serde(serialize_with = "cainome::cairo_serde::serialize_as_hex_t2")] | ||
| pub nonce: (starknet::core::types::Felt, u128), | ||
| #[serde(serialize_with = "cainome::cairo_serde::serialize_as_hex")] |
There was a problem hiding this comment.
Correct the placement of the serde attribute
The #[serde(serialize_with = "cainome::cairo_serde::serialize_as_hex")] attribute on line 235 should be placed immediately before the field it annotates (execute_after). Currently, it's placed after the nonce field, which can lead to incorrect serialization behavior.
Apply this diff to correct the attribute placement:
pub nonce: (starknet::core::types::Felt, u128),
-#[serde(serialize_with = "cainome::cairo_serde::serialize_as_hex")]
pub execute_after: u64,
+#[serde(serialize_with = "cainome::cairo_serde::serialize_as_hex")]Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/account_sdk/src/abigen/erc_20.rs (2)
Line range hint
115-148: Inconsistent method naming conventions inErc20<A>structThe methods
balanceOfandtotalSupplyare usingcamelCase, whereas Rust conventionally usessnake_casefor function and method names. Additionally, you have bothbalanceOfandbalance_of, as well astotalSupplyandtotal_supply, which can lead to confusion and redundancy.Please consider renaming
balanceOftobalance_ofandtotalSupplytototal_supply, and remove any duplicate methods to adhere to Rust's naming conventions and improve code clarity.
Line range hint
285-318: Inconsistent method naming conventions inErc20Reader<P>structSimilar to the
Erc20<A>struct, theErc20Reader<P>struct contains methodsbalanceOfandtotalSupplythat do not follow Rust'ssnake_casenaming convention. There are also duplicate methods with bothcamelCaseandsnake_casenames.Please standardize the method names to use
snake_caseand eliminate duplicates to maintain consistency and improve readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (14)
Cargo.tomlis excluded by none and included by noneScarb.tomlis excluded by none and included by nonepackages/account-wasm/src/account.rsis excluded by none and included by nonepackages/account-wasm/src/session.rsis excluded by none and included by nonepackages/account_sdk/artifacts/controller.latest.compiled_contract_class.jsonis excluded by none and included by nonepackages/account_sdk/artifacts/controller.latest.contract_class.jsonis excluded by none and included by nonepackages/account_sdk/cmd/bench.rsis excluded by none and included by nonepackages/account_sdk/src/account/outside_execution.rsis excluded by none and included by nonepackages/account_sdk/src/artifacts.rsis excluded by none and included by nonepackages/account_sdk/src/controller.rsis excluded by none and included by nonepackages/account_sdk/src/execute_from_outside.rsis excluded by none and included by nonepackages/account_sdk/src/execute_from_outside_test.rsis excluded by none and included by nonepackages/account_sdk/src/tests/runners/test_data/session_execute_outside.jsonis excluded by none and included by nonepackages/account_sdk/src/tests/runners/test_data/webauthn_execute_outside.jsonis excluded by none and included by none
📒 Files selected for processing (2)
- packages/account_sdk/src/abigen/controller.rs (10 hunks)
- packages/account_sdk/src/abigen/erc_20.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/account_sdk/src/abigen/controller.rs (3)
Line range hint
1-3067: Summary of changes and potential impactThis update primarily focuses on changing the
noncefield in theOutsideExecutionstruct from(Felt, Felt)to(Felt, u128). This change has cascading effects throughout the file, including:
- Updated serialization and deserialization logic for the
noncefield.- Modified method signatures and return types for nonce-related functions.
- Potential impacts on nonce generation, processing, and storage throughout the system.
While the changes appear consistent within this file, it's crucial to ensure that:
- All parts of the system interacting with nonces have been updated to handle the new type.
- Backward compatibility has been considered, especially for stored data or external integrations.
- Performance implications of the type change have been evaluated.
Additionally, the global disabling of Clippy lints and warnings should be reconsidered to maintain code quality.
To ensure a smooth transition, consider running integration tests and reviewing any external systems or contracts that interact with this code.
2267-2267: Verify callers of updated nonce-related methodsThe following methods have been updated to use
u128instead ofFeltfor the nonce:
get_outside_execution_v3_channel_nonceis_valid_outside_execution_v3_nonceEnsure that all callers of these methods have been updated to handle the new
u128type correctly. This change might affect:
- How nonce values are generated and processed
- Any arithmetic or comparison operations performed on nonce values
- Storage and retrieval of nonce values
To verify the impact of this change, run the following script:
#!/bin/bash # Description: Find all calls to the updated nonce-related methods echo "Calls to get_outside_execution_v3_channel_nonce:" rg -p 'get_outside_execution_v3_channel_nonce' packages/account_sdk/src echo "\nCalls to is_valid_outside_execution_v3_nonce:" rg -p 'is_valid_outside_execution_v3_nonce' packages/account_sdk/srcAlso applies to: 2372-2378, 2916-2916, 3021-3027
233-235:⚠️ Potential issueVerify all usages of the
noncefield after type changeThe
noncefield in theOutsideExecutionstruct has been changed from(Felt, Felt)to(Felt, u128). This change may have significant implications:
- Backward Compatibility: Ensure this change doesn't break existing functionality or stored data.
- Performance: Verify if this change affects performance, especially in serialization/deserialization.
- Usage: Check all places where
nonceis used to ensure they're updated to handle the new type correctly.To verify the impact of this change, run the following script:
✅ Verification successful
All usages of the
noncefield have been successfully updated to the new type(Felt, u128). No instances of the old type(Felt, Felt)were found, ensuring consistency and preventing potential type mismatches.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of the OutsideExecution struct and its nonce field # Search for OutsideExecution struct usage echo "OutsideExecution struct usage:" rg -p 'OutsideExecution' packages/account_sdk/src # Search for nonce field access echo "\nNonce field access:" rg -p 'OutsideExecution.*\.nonce' packages/account_sdk/srcLength of output: 1009
Script:
#!/bin/bash # Description: Refined search for usages of the OutsideExecution struct and its nonce field # Search for OutsideExecution struct definitions echo "Searching for OutsideExecution struct definitions:" rg 'struct\s+OutsideExecution' packages/account_sdk/src # Search for nonce field within OutsideExecution echo "\nSearching for nonce field within OutsideExecution:" rg 'nonce\s*:' packages/account_sdk/srcLength of output: 1600
Script:
#!/bin/bash # Description: Identify usages of the nonce field with the old type (Felt, Felt) # Search for nonce field initialized or defined with (Felt, Felt) echo "Searching for nonce field with type (Felt, Felt):" rg 'nonce\s*:\s*\(starknet::core::types::Felt,\s*starknet::core::types::Felt\)' packages/account_sdk/src # Search for nonce field assignments with (Felt, Felt) echo "\nSearching for nonce assignments with (Felt, Felt):" rg 'nonce\s*=\s*\(.*starknet::core::types::Felt,.*starknet::core::types::Felt\)' packages/account_sdk/src # Search for function signatures using nonce with the old type echo "\nSearching for function signatures using nonce with type (Felt, Felt):" rg 'fn\s+\w+\s*\(.*nonce\s*:\s*\(starknet::core::types::Felt,\s*starknet::core::types::Felt\).*?\)' packages/account_sdk/srcLength of output: 718
packages/account_sdk/src/abigen/erc_20.rs (1)
5-6: Previous comment about suppressing Clippy lints still appliesThe use of
#![allow(clippy::all)]and#![allow(warnings)]suppresses all Clippy lints and compiler warnings for the entire module. This can hide potential issues and decrease code quality. Please consider addressing warnings individually or allowing specific lints as necessary.
| #![allow(clippy::all)] | ||
| #![allow(warnings)] |
There was a problem hiding this comment.
Reconsider globally disabling all Clippy lints and warnings
Disabling all Clippy lints and warnings can mask potential issues and reduce code quality. It's recommended to:
- Address specific warnings individually.
- If necessary, disable only specific lints that are not applicable to this auto-generated code.
- Consider using
#[allow(...)]attributes on specific items rather than the entire file.
This approach will help maintain code quality and catch potential issues early.
Summary by CodeRabbit
New Features
Bug Fixes