-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(torii): model upgrades #2637
Changes from 3 commits
b1f3e35
fa71dbd
60a870d
62750f6
6313f44
f7fd6ae
e85bdd0
d641ba0
3336c4a
3742676
b3ee109
0d26d43
cbc39c7
14ffc8e
ddd9921
cf9c45c
3925cce
ae8bf42
210ac31
d86fcc4
ab855dd
5d57e6d
b04e848
e3d8a7b
b7f3264
87857c2
9b5d624
9ac0d21
a8d6737
d2c7696
255d4fb
6235080
612fc4b
0dbec7e
551c6bd
cea8102
155033d
4e921c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,6 +117,41 @@ where | |
}) | ||
} | ||
|
||
pub async fn new_with_selector( | ||
selector: Felt, | ||
world: &'a WorldContractReader<P>, | ||
) -> Result<ModelRPCReader<'a, P>, ModelError> { | ||
// Events are also considered like models from a off-chain perspective. They both have | ||
// introspection and convey type information. | ||
let (contract_address, class_hash) = | ||
match world.resource(&selector).block_id(world.block_id).call().await? { | ||
abigen::world::Resource::Model((address, hash)) => (address, hash), | ||
abigen::world::Resource::Event((address, hash)) => (address, hash), | ||
_ => return Err(ModelError::ModelNotFound), | ||
}; | ||
|
||
// World Cairo contract won't raise an error in case of unknown/unregistered | ||
// model so raise an error here in case of zero address. | ||
if contract_address == ContractAddress(Felt::ZERO) { | ||
return Err(ModelError::ModelNotFound); | ||
} | ||
|
||
let model_reader = ModelContractReader::new(contract_address.into(), world.provider()); | ||
let tag = model_reader.dojo_name().call().await?; | ||
// Safe to unwrap, since it's coming from the chain. | ||
let (namespace, name) = naming::split_tag(&tag.to_string().unwrap()).unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification 🛠️ Refactor suggestion Ohayo! The unwrap is indeed unsafe here, sensei! Looking at the codebase, I found that proper error handling for The code should follow the established pattern: let (namespace, name) = naming::split_tag(&tag.to_string()
.map_err(|e| ModelError::TagError(format!("Invalid tag string: {}", e)))?
).map_err(|e| ModelError::TagError(e.to_string()))?; Key evidence:
🔗 Analysis chainOhayo! Consider handling potential unwrap failure more gracefully. The unwrap on line 142 assumes the tag will always be valid, with a comment stating it's safe because it's "coming from the chain". However, for better robustness and maintainability, consider handling the potential failure case explicitly. Consider this safer approach: - let (namespace, name) = naming::split_tag(&tag.to_string().unwrap()).unwrap();
+ let tag_str = tag.to_string().map_err(|e| ModelError::TagError(format!("Invalid tag string: {}", e)))?;
+ let (namespace, name) = naming::split_tag(&tag_str)
+ .map_err(|e| ModelError::TagError(format!("Invalid tag format: {}", e)))?; Let me analyze the usage of 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any existing error handling patterns for tag parsing
rg -A 2 "split_tag|to_string" --type rust
Length of output: 297611 Script: #!/bin/bash
# Search for error handling patterns around split_tag and tag parsing
ast-grep --pattern 'naming::split_tag($tag).map_err($err)'
Length of output: 60 Script: #!/bin/bash
# Search for any validation or safety checks around tag parsing
rg -A 3 "split_tag" --type rust
Length of output: 4654 Script: #!/bin/bash
# Look for any error types or handling related to tag parsing
rg "TagError" --type rust
Length of output: 215 Script: #!/bin/bash
# Check the implementation of split_tag function
ast-grep --pattern 'fn split_tag($tag) -> Result<($_, $_)>'
Length of output: 61 |
||
|
||
Ok(Self { | ||
namespace: namespace.into(), | ||
name: name.into(), | ||
world_reader: world, | ||
class_hash, | ||
contract_address: contract_address.into(), | ||
selector, | ||
model_reader, | ||
}) | ||
} | ||
|
||
pub async fn entity_storage(&self, keys: &[Felt]) -> Result<Vec<Felt>, ModelError> { | ||
// As the dojo::model::Layout type has been pasted | ||
// in both `model` and `world` ABI by abigen, the compiler sees both types | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ where | |
P: Provider + Send + Sync + std::fmt::Debug, | ||
{ | ||
fn event_key(&self) -> String { | ||
"EventRegistered".to_string() | ||
"EventUpgraded".to_string() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohayo! Critical mismatch between event key and event handling, sensei! There appears to be an inconsistency in the event handling logic:
This mismatch could lead to runtime issues where events are not properly processed. Consider updating the event matching to align with the new event key: - let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <RegisterEventProcessor as EventProcessor<P>>::event_key(self)
- )
- }) {
- WorldEvent::EventRegistered(e) => e,
- _ => {
- unreachable!()
- }
- };
+ let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
+ panic!(
+ "Expected {} event to be well formed.",
+ <RegisterEventProcessor as EventProcessor<P>>::event_key(self)
+ )
+ }) {
+ WorldEvent::EventUpgraded(e) => e,
+ _ => {
+ unreachable!()
+ }
+ }; Also applies to: 41-45 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Ohayo! The event key change requires updates in multiple places, sensei. The change from "EventRegistered" to "EventUpgraded" will break existing functionality as "EventRegistered" is heavily used across the codebase:
This change should either:
🔗 Analysis chainOhayo! Verify event key change impact. The event key change from "EventRegistered" to "EventUpgraded" needs careful verification to ensure all consumers handle this correctly. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any direct string references to the old event key
rg "EventRegistered" --type rust
# Search for event key usage in match patterns
ast-grep --pattern 'match $_ {
$$$
"EventRegistered" => $$$
$$$
}'
Length of output: 3355 |
||
} | ||
|
||
// We might not need this anymore, since we don't have fallback and all world events must | ||
|
@@ -71,7 +71,7 @@ where | |
target: LOG_TARGET, | ||
namespace = %namespace, | ||
name = %name, | ||
"Registered event." | ||
"Upgraded event." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Ohayo! Found inconsistent log messages that need updating, sensei! The codebase contains similar log messages that should be updated for consistency:
These messages should be updated to use "Upgraded" terminology to maintain consistency with the changes in this PR. 🔗 Analysis chainVerify log message consistency across the codebase, sensei! The log messages have been updated to reflect the upgrade terminology. Let's ensure this change is consistent across the codebase. Also applies to: 86-86 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any remaining "Registered event" or similar phrases that might need updating
rg -i "registered (event|model)" --type rust
Length of output: 392 |
||
); | ||
|
||
debug!( | ||
|
@@ -83,7 +83,7 @@ where | |
contract_address = ?event.address, | ||
packed_size = %packed_size, | ||
unpacked_size = %unpacked_size, | ||
"Registered model content." | ||
"Upgraded event content." | ||
); | ||
|
||
db.register_model( | ||
|
@@ -95,6 +95,7 @@ where | |
packed_size, | ||
unpacked_size, | ||
block_timestamp, | ||
true, | ||
) | ||
.await?; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,7 @@ where | |
packed_size, | ||
unpacked_size, | ||
block_timestamp, | ||
false, | ||
) | ||
.await?; | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,102 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
use anyhow::{Error, Ok, Result}; | ||||||||||||||||||||||||||||||||||||||||||||||||||
use async_trait::async_trait; | ||||||||||||||||||||||||||||||||||||||||||||||||||
use dojo_world::contracts::abigen::world::Event as WorldEvent; | ||||||||||||||||||||||||||||||||||||||||||||||||||
use dojo_world::contracts::model::ModelReader; | ||||||||||||||||||||||||||||||||||||||||||||||||||
use dojo_world::contracts::world::WorldContractReader; | ||||||||||||||||||||||||||||||||||||||||||||||||||
use starknet::core::types::Event; | ||||||||||||||||||||||||||||||||||||||||||||||||||
use starknet::providers::Provider; | ||||||||||||||||||||||||||||||||||||||||||||||||||
use tracing::{debug, info}; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
use super::EventProcessor; | ||||||||||||||||||||||||||||||||||||||||||||||||||
use crate::sql::Sql; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_event"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Inconsistent Ohayo, sensei! Noticed that the pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_event"; Since this module is Apply this diff to fix it: -pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_event";
+pub(crate) const LOG_TARGET: &str = "torii_core::processors::upgrade_event"; |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
#[derive(Default, Debug)] | ||||||||||||||||||||||||||||||||||||||||||||||||||
pub struct RegisterEventProcessor; | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Ohayo sensei! Consider renaming the struct and log target for consistency The struct Apply this diff to rename the struct and update the log target: 13 -pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_event";
+pub(crate) const LOG_TARGET: &str = "torii_core::processors::upgrade_event";
16 -pub struct RegisterEventProcessor;
+pub struct UpgradeEventProcessor; Also, update the implementation accordingly: 19 -impl<P> EventProcessor<P> for RegisterEventProcessor
+impl<P> EventProcessor<P> for UpgradeEventProcessor 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
#[async_trait] | ||||||||||||||||||||||||||||||||||||||||||||||||||
impl<P> EventProcessor<P> for RegisterEventProcessor | ||||||||||||||||||||||||||||||||||||||||||||||||||
where | ||||||||||||||||||||||||||||||||||||||||||||||||||
P: Provider + Send + Sync + std::fmt::Debug, | ||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||
fn event_key(&self) -> String { | ||||||||||||||||||||||||||||||||||||||||||||||||||
"EventUpgraded".to_string() | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// We might not need this anymore, since we don't have fallback and all world events must | ||||||||||||||||||||||||||||||||||||||||||||||||||
// be handled. | ||||||||||||||||||||||||||||||||||||||||||||||||||
fn validate(&self, _event: &Event) -> bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||
true | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
async fn process( | ||||||||||||||||||||||||||||||||||||||||||||||||||
&self, | ||||||||||||||||||||||||||||||||||||||||||||||||||
world: &WorldContractReader<P>, | ||||||||||||||||||||||||||||||||||||||||||||||||||
db: &mut Sql, | ||||||||||||||||||||||||||||||||||||||||||||||||||
_block_number: u64, | ||||||||||||||||||||||||||||||||||||||||||||||||||
block_timestamp: u64, | ||||||||||||||||||||||||||||||||||||||||||||||||||
_event_id: &str, | ||||||||||||||||||||||||||||||||||||||||||||||||||
event: &Event, | ||||||||||||||||||||||||||||||||||||||||||||||||||
) -> Result<(), Error> { | ||||||||||||||||||||||||||||||||||||||||||||||||||
// Torii version is coupled to the world version, so we can expect the event to be well | ||||||||||||||||||||||||||||||||||||||||||||||||||
// formed. | ||||||||||||||||||||||||||||||||||||||||||||||||||
let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
panic!( | ||||||||||||||||||||||||||||||||||||||||||||||||||
"Expected {} event to be well formed.", | ||||||||||||||||||||||||||||||||||||||||||||||||||
<RegisterEventProcessor as EventProcessor<P>>::event_key(self) | ||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||
}) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohayo sensei! Avoid using Using Consider replacing the 44 -let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
45 - panic!(
46 - "Expected {} event to be well formed.",
47 - <RegisterEventProcessor as EventProcessor<P>>::event_key(self)
48 - )
49 -}) {
+let event = match WorldEvent::try_from(event) {
+ Ok(WorldEvent::EventUpgraded(e)) => e,
+ Ok(_) => {
+ unreachable!()
+ },
+ Err(_) => {
+ return Err(Error::msg("Failed to parse EventUpgraded event"));
+ }
+}; This change ensures that errors are properly propagated without causing a panic.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid panicking on event parsing failure Ohayo, sensei! Using Apply this diff: -let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <RegisterEventProcessor as EventProcessor<P>>::event_key(self)
- )
-}) {
+let event = match WorldEvent::try_from(event)? { Adjust the match arms and ensure that any errors are properly propagated.
|
||||||||||||||||||||||||||||||||||||||||||||||||||
WorldEvent::EventUpgraded(e) => e, | ||||||||||||||||||||||||||||||||||||||||||||||||||
_ => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
unreachable!() | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+52
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Handle unexpected event types explicitly Ohayo, sensei! Instead of using Apply this diff: -_ => {
- unreachable!()
-}
+_ => {
+ return Err(anyhow!("Unexpected event type received in UpgradeEventProcessor."));
+}
|
||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// Called model here by language, but it's an event. Torii rework will make clear | ||||||||||||||||||||||||||||||||||||||||||||||||||
// distinction. | ||||||||||||||||||||||||||||||||||||||||||||||||||
let model = world.model_reader_with_selector(event.selector).await?; | ||||||||||||||||||||||||||||||||||||||||||||||||||
let namespace = model.namespace(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
let name = model.name(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
let schema = model.schema().await?; | ||||||||||||||||||||||||||||||||||||||||||||||||||
let layout = model.layout().await?; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// Events are never stored onchain, hence no packing or unpacking. | ||||||||||||||||||||||||||||||||||||||||||||||||||
let unpacked_size: u32 = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||
let packed_size: u32 = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
info!( | ||||||||||||||||||||||||||||||||||||||||||||||||||
target: LOG_TARGET, | ||||||||||||||||||||||||||||||||||||||||||||||||||
namespace = %namespace, | ||||||||||||||||||||||||||||||||||||||||||||||||||
name = %name, | ||||||||||||||||||||||||||||||||||||||||||||||||||
"Registered event." | ||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
debug!( | ||||||||||||||||||||||||||||||||||||||||||||||||||
target: LOG_TARGET, | ||||||||||||||||||||||||||||||||||||||||||||||||||
name, | ||||||||||||||||||||||||||||||||||||||||||||||||||
schema = ?schema, | ||||||||||||||||||||||||||||||||||||||||||||||||||
layout = ?layout, | ||||||||||||||||||||||||||||||||||||||||||||||||||
class_hash = ?event.class_hash, | ||||||||||||||||||||||||||||||||||||||||||||||||||
contract_address = ?event.address, | ||||||||||||||||||||||||||||||||||||||||||||||||||
packed_size = %packed_size, | ||||||||||||||||||||||||||||||||||||||||||||||||||
unpacked_size = %unpacked_size, | ||||||||||||||||||||||||||||||||||||||||||||||||||
"Registered model content." | ||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
db.register_model( | ||||||||||||||||||||||||||||||||||||||||||||||||||
&namespace, | ||||||||||||||||||||||||||||||||||||||||||||||||||
schema, | ||||||||||||||||||||||||||||||||||||||||||||||||||
layout, | ||||||||||||||||||||||||||||||||||||||||||||||||||
event.class_hash.into(), | ||||||||||||||||||||||||||||||||||||||||||||||||||
event.address.into(), | ||||||||||||||||||||||||||||||||||||||||||||||||||
packed_size, | ||||||||||||||||||||||||||||||||||||||||||||||||||
unpacked_size, | ||||||||||||||||||||||||||||||||||||||||||||||||||
block_timestamp, | ||||||||||||||||||||||||||||||||||||||||||||||||||
false, | ||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Ohayo sensei! The Looking at the codebase, I found that:
Since this processor handles upgrade events, it should align with 🔗 Analysis chainOhayo sensei! Verify the When registering the model in the database, the Run the following script to check how This will help confirm whether the 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the usage of the `is_upgrade` flag in `register_model` calls.
# Search for calls to `register_model` and display subsequent lines
rg 'db\.register_model\(' -A 10
Length of output: 6644 |
||||||||||||||||||||||||||||||||||||||||||||||||||
.await?; | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Set Ohayo, sensei! When registering a model during an upgrade event, the Apply this diff: block_timestamp,
- false,
+ true,
) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,98 @@ | ||||||||||||||||||||||||||||||||||||||||||
use anyhow::{Error, Ok, Result}; | ||||||||||||||||||||||||||||||||||||||||||
use async_trait::async_trait; | ||||||||||||||||||||||||||||||||||||||||||
use dojo_world::contracts::{abigen::world::Event as WorldEvent, model::ModelReader}; | ||||||||||||||||||||||||||||||||||||||||||
use dojo_world::contracts::world::WorldContractReader; | ||||||||||||||||||||||||||||||||||||||||||
use starknet::core::types::Event; | ||||||||||||||||||||||||||||||||||||||||||
use starknet::providers::Provider; | ||||||||||||||||||||||||||||||||||||||||||
use tracing::{debug, info}; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
use super::EventProcessor; | ||||||||||||||||||||||||||||||||||||||||||
use crate::sql::Sql; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_model"; | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Inconsistent Ohayo, sensei! The |
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
#[derive(Default, Debug)] | ||||||||||||||||||||||||||||||||||||||||||
pub struct RegisterModelProcessor; | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Rename struct to reflect its functionality Ohayo, sensei! The struct is named |
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
#[async_trait] | ||||||||||||||||||||||||||||||||||||||||||
impl<P> EventProcessor<P> for RegisterModelProcessor | ||||||||||||||||||||||||||||||||||||||||||
where | ||||||||||||||||||||||||||||||||||||||||||
P: Provider + Send + Sync + std::fmt::Debug, | ||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||
fn event_key(&self) -> String { | ||||||||||||||||||||||||||||||||||||||||||
"ModelUpgraded".to_string() | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// We might not need this anymore, since we don't have fallback and all world events must | ||||||||||||||||||||||||||||||||||||||||||
// be handled. | ||||||||||||||||||||||||||||||||||||||||||
fn validate(&self, _event: &Event) -> bool { | ||||||||||||||||||||||||||||||||||||||||||
true | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
async fn process( | ||||||||||||||||||||||||||||||||||||||||||
&self, | ||||||||||||||||||||||||||||||||||||||||||
world: &WorldContractReader<P>, | ||||||||||||||||||||||||||||||||||||||||||
db: &mut Sql, | ||||||||||||||||||||||||||||||||||||||||||
_block_number: u64, | ||||||||||||||||||||||||||||||||||||||||||
block_timestamp: u64, | ||||||||||||||||||||||||||||||||||||||||||
_event_id: &str, | ||||||||||||||||||||||||||||||||||||||||||
event: &Event, | ||||||||||||||||||||||||||||||||||||||||||
) -> Result<(), Error> { | ||||||||||||||||||||||||||||||||||||||||||
// Torii version is coupled to the world version, so we can expect the event to be well | ||||||||||||||||||||||||||||||||||||||||||
// formed. | ||||||||||||||||||||||||||||||||||||||||||
let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | ||||||||||||||||||||||||||||||||||||||||||
panic!( | ||||||||||||||||||||||||||||||||||||||||||
"Expected {} event to be well formed.", | ||||||||||||||||||||||||||||||||||||||||||
<RegisterModelProcessor as EventProcessor<P>>::event_key(self) | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
}) { | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid panicking; handle event parsing errors gracefully Ohayo, sensei! The current implementation panics if -let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <RegisterModelProcessor as EventProcessor<P>>::event_key(self)
- )
-}) {
- WorldEvent::ModelUpgraded(e) => e,
- _ => {
- unreachable!()
- }
-};
+let event = match WorldEvent::try_from(event)? {
+ WorldEvent::ModelUpgraded(e) => e,
+ _ => {
+ return Err(Error::msg(format!(
+ "Unexpected event type for key {}.",
+ self.event_key()
+ )));
+ }
+}; This change ensures that parsing errors are properly handled without causing a panic.
|
||||||||||||||||||||||||||||||||||||||||||
WorldEvent::ModelUpgraded(e) => e, | ||||||||||||||||||||||||||||||||||||||||||
_ => { | ||||||||||||||||||||||||||||||||||||||||||
unreachable!() | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid Ohayo, sensei! Using Apply this diff to return an error instead of panicking: -let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <RegisterModelProcessor as EventProcessor<P>>::event_key(self)
- )
-}) {
- WorldEvent::ModelUpgraded(e) => e,
- _ => {
- unreachable!()
- }
-};
+let event = match WorldEvent::try_from(event)? {
+ WorldEvent::ModelUpgraded(e) => e,
+ other_event => {
+ return Err(Error::msg(format!(
+ "Expected ModelUpgraded event, but received {:?}",
+ other_event
+ )));
+ }
+}; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
let model = world.model_reader_with_selector(event.selector).await?; | ||||||||||||||||||||||||||||||||||||||||||
let namespace = model.namespace(); | ||||||||||||||||||||||||||||||||||||||||||
let name = model.name(); | ||||||||||||||||||||||||||||||||||||||||||
let schema = model.schema().await?; | ||||||||||||||||||||||||||||||||||||||||||
let layout = model.layout().await?; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
let unpacked_size: u32 = model.unpacked_size().await?; | ||||||||||||||||||||||||||||||||||||||||||
let packed_size: u32 = model.packed_size().await?; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
info!( | ||||||||||||||||||||||||||||||||||||||||||
target: LOG_TARGET, | ||||||||||||||||||||||||||||||||||||||||||
namespace = %namespace, | ||||||||||||||||||||||||||||||||||||||||||
name = %name, | ||||||||||||||||||||||||||||||||||||||||||
"Upgraded model." | ||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
debug!( | ||||||||||||||||||||||||||||||||||||||||||
target: LOG_TARGET, | ||||||||||||||||||||||||||||||||||||||||||
name = %name, | ||||||||||||||||||||||||||||||||||||||||||
schema = ?schema, | ||||||||||||||||||||||||||||||||||||||||||
layout = ?layout, | ||||||||||||||||||||||||||||||||||||||||||
class_hash = ?event.class_hash, | ||||||||||||||||||||||||||||||||||||||||||
contract_address = ?event.address, | ||||||||||||||||||||||||||||||||||||||||||
packed_size = %packed_size, | ||||||||||||||||||||||||||||||||||||||||||
unpacked_size = %unpacked_size, | ||||||||||||||||||||||||||||||||||||||||||
"Upgraded model content." | ||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
db.register_model( | ||||||||||||||||||||||||||||||||||||||||||
&namespace, | ||||||||||||||||||||||||||||||||||||||||||
schema, | ||||||||||||||||||||||||||||||||||||||||||
layout, | ||||||||||||||||||||||||||||||||||||||||||
event.class_hash.into(), | ||||||||||||||||||||||||||||||||||||||||||
event.address.into(), | ||||||||||||||||||||||||||||||||||||||||||
packed_size, | ||||||||||||||||||||||||||||||||||||||||||
unpacked_size, | ||||||||||||||||||||||||||||||||||||||||||
block_timestamp, | ||||||||||||||||||||||||||||||||||||||||||
true, | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
.await?; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
🛠️ Refactor suggestion
Ohayo sensei! After reviewing the codebase, I can now provide a clear assessment of the tag parsing situation.
The unwrap calls are actually not safe here, sensei!
While the comment suggests the data is safe because it comes from the chain, the codebase shows proper error handling patterns for tag parsing through
ModelError::TagError
. We should follow this pattern:model_reader.dojo_name()
call is already properly handled with?
tag.to_string()
andnaming::split_tag()
unwraps should be handled with proper error propagationHere's the safer approach:
This matches the error handling patterns seen in the codebase, particularly in
world.rs
where tag parsing errors are properly propagated usingModelError::TagError
.🔗 Analysis chain
Consider more robust error handling for tag parsing, sensei!
While the comment suggests the unwraps are safe because the data comes from the chain, it might be better to handle potential errors gracefully:
Consider this safer approach:
Let me check the codebase further to understand how the tag parsing is handled in other places.
Let me check the error handling in the codebase further to understand how tag errors are handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 187299
Script:
Length of output: 40858
Script:
Length of output: 10819