Upgrade contract to Cairo 2.7 #549
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe updates primarily focus on refining event handling and serialization processes in the Changes
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (4)
Files selected for processing (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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
Outside diff range, codebase verification and nitpick comments (1)
packages/account_sdk/src/abigen/mod.rs (1)
6-23: MissingControllercontract definition in JSON files.The
Controllercontract definition is not found in the JSON files, which might lead to issues with theabigen!macro invocation. Verify the existence and correctness of the contract definition in the JSON files to ensure the macro functions as intended.
- Ensure that the
controller.contract_class.jsonfile contains the correctControllercontract definition.Analysis chain
Ensure correctness of
abigen!macro invocation.The
abigen!macro is used to generate bindings for a smart contract. Verify that the contract name, JSON path, type aliases, and derived traits are correct and consistent with the rest of the codebase.Run the following script to verify the correctness of the
abigen!macro invocation:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `abigen!` macro invocation. # Test: Check if the specified JSON file exists. Expect: The file should exist. fd 'controller.contract_class.json' --exec echo "JSON file exists: {}" # Test: Search for the `Controller` contract definition. Expect: The contract should be defined. rg --type json '"contract_name": "Controller"' -A 5 # Test: Verify the type aliases. Expect: The aliases should match the event definitions. rg --type rust 'pub enum Event' -A 5Length of output: 2194
Script:
#!/bin/bash # Description: Further investigate the `Controller` contract definition. # Test: Search for `Controller` contract definition in all JSON files. rg '"Controller"' --type json -A 5 # Test: Search for `Controller` contract definition in all Rust files. rg 'Controller' --type rust -A 5Length of output: 24621
| ReentrancyGuardEvent(Event), | ||
| SessionEvent(SessionEvent), | ||
| ExternalOwnersEvent(ExternalOwnersEvent), | ||
| ExecuteFromOutsideEvents(OutsideExecutionEvent), | ||
| DelegateAccountEvents(DelegateAccountEvent), | ||
| SRC5Events(Src5ComponentEvent), | ||
| UpgradeableEvent(UpgradeEvent), | ||
| UpgradeableEvent(Event), |
There was a problem hiding this comment.
Remove redundant ReentrancyGuardEvent and UpgradeableEvent variants.
The ReentrancyGuardEvent and UpgradeableEvent variants in the Event enum are redundant and should be removed to simplify the event handling logic.
Use this diff to remove the redundant variants:
pub enum Event {
TransactionExecuted(TransactionExecuted),
MultipleOwnersEvent(MultipleOwnersEvent),
- ReentrancyGuardEvent(Event),
SessionEvent(SessionEvent),
ExternalOwnersEvent(ExternalOwnersEvent),
ExecuteFromOutsideEvents(OutsideExecutionEvent),
DelegateAccountEvents(DelegateAccountEvent),
SRC5Events(Src5ComponentEvent),
- UpgradeableEvent(Event),
}Also applies to: 1050-1062, 1083-1119, 1149-1191
| return Ok(Event::UpgradeableEvent(Event::Upgraded(Upgraded { class_hash }))); | ||
| } | ||
| Err(format!("Could not match any event from keys {:?}", event.keys)) | ||
| } |
There was a problem hiding this comment.
Consolidate event handling logic.
The try_from implementation for Event should consolidate event handling logic by removing redundant checks for ReentrancyGuardEvent and UpgradeableEvent.
Use this diff to simplify the try_from implementation:
if selector
== starknet::core::utils::get_selector_from_name("Upgraded")
.unwrap_or_else(|_| panic!("Invalid selector for {}", "Upgraded"))
{
let mut key_offset = 0 + 1;
let mut data_offset = 0;
let class_hash = match cainome::cairo_serde::ClassHash::cairo_deserialize(
&event.data,
data_offset,
) {
Ok(v) => v,
Err(e) => {
return Err(
format!(
"Could not deserialize field {} for {}: {:?}", "class_hash",
"Upgraded", e
),
);
}
};
data_offset
+= cainome::cairo_serde::ClassHash::cairo_serialized_size(&class_hash);
return Ok(Event::Upgraded(Upgraded { class_hash }));
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
packages/account_sdk/src/abigen/controller.rs (1)
1030-1036: Incomplete Refactoring ofEventEnumThe
ReentrancyGuardEventandUpgradeableEventvariants are still present in theEventenum incontroller.rs, which contradicts the intended refactoring. These variants should be removed, and theUpgradedvariant should be used consistently throughout the codebase.
- File:
packages/account_sdk/src/abigen/controller.rs- Lines: 1030-1036
Please ensure that all instances of the
Eventenum are updated to reflect these changes.Analysis chain
LGTM! Verify
Eventenum usage in the codebase.The removal of
ReentrancyGuardEventandUpgradeableEventvariants, and the addition of theUpgradedvariant, simplifies the event handling logic.Ensure that all instances of the
Eventenum are updated to reflect these changes.Run the following script to verify the enum usage:
Also applies to: 1500-1503, 1551-1553
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instances of `Event` reflect the removal of `ReentrancyGuardEvent` and `UpgradeableEvent`, and the addition of `Upgraded`. # Test: Search for the enum usage. Expect: No references to `ReentrancyGuardEvent` and `UpgradeableEvent`, and correct usage of `Upgraded`. rg --type rust -A 5 $'Event'Length of output: 87361
Changes also made here: cartridge-gg/argent-contracts-starknet#2
Summary by CodeRabbit
New Features
Eventenum, introducing the newUpgradedvariant.WebauthnSignaturestruct for streamlined serialization processes.Improvements
abigen!macro for improved interaction with smart contract events, enhancing usability and maintainability.These changes significantly enhance the efficiency and clarity of the account SDK, benefiting developers working with event handling and smart contracts.