Skip to content
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

Merged
merged 38 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
b1f3e35
feat(torii): model upgrades
Larkooo Nov 5, 2024
fa71dbd
upgrade processor for event and model
Larkooo Nov 6, 2024
60a870d
Merge branch 'main' into model-upgrade
Larkooo Nov 6, 2024
62750f6
fix processors
Larkooo Nov 6, 2024
6313f44
fix: model upgrade
Larkooo Nov 6, 2024
f7fd6ae
wrap up model upgrade
Larkooo Nov 6, 2024
e85bdd0
ftm
Larkooo Nov 6, 2024
d641ba0
clippy
Larkooo Nov 6, 2024
3336c4a
fmt
Larkooo Nov 6, 2024
3742676
refactor: diff
Larkooo Nov 7, 2024
b3ee109
fmtg
Larkooo Nov 7, 2024
0d26d43
fix: set model cache
Larkooo Nov 7, 2024
cbc39c7
fix add model members
Larkooo Nov 7, 2024
14ffc8e
fix
Larkooo Nov 7, 2024
ddd9921
feat: shared cache between grpc & engine and fix partial deser
Larkooo Nov 8, 2024
cf9c45c
fix: test and fmt
Larkooo Nov 8, 2024
3925cce
Merge remote-tracking branch 'upstream/main' into model-upgrade
Larkooo Nov 10, 2024
ae8bf42
fix
Larkooo Nov 10, 2024
210ac31
primitives not option
Larkooo Nov 12, 2024
d86fcc4
fix: enums
Larkooo Nov 13, 2024
ab855dd
Revert "fix: enums"
Larkooo Nov 13, 2024
5d57e6d
Revert "primitives not option"
Larkooo Nov 13, 2024
b04e848
fix enum sql value
Larkooo Nov 13, 2024
e3d8a7b
Merge remote-tracking branch 'upstream/main' into model-upgrade
Larkooo Nov 13, 2024
b7f3264
main
Larkooo Nov 13, 2024
87857c2
remove prints & format
Larkooo Nov 13, 2024
9b5d624
fix quer ytest
Larkooo Nov 13, 2024
9ac0d21
fix: bool
Larkooo Nov 14, 2024
a8d6737
fix: ararys
Larkooo Nov 14, 2024
d2c7696
fmt
Larkooo Nov 14, 2024
255d4fb
fix: map row to ty
Larkooo Nov 14, 2024
6235080
fix: enum
Larkooo Nov 14, 2024
612fc4b
fix: enum
Larkooo Nov 14, 2024
0dbec7e
fmt
Larkooo Nov 14, 2024
551c6bd
fix: primitive len
Larkooo Nov 14, 2024
cea8102
Revert "fix: primitive len"
Larkooo Nov 14, 2024
155033d
refactotr: dont use modelr eader block
Larkooo Nov 14, 2024
4e921c3
Revert "Revert "fix: primitive len""
Larkooo Nov 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions crates/dojo/world/src/contracts/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link

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:

  1. The model_reader.dojo_name() call is already properly handled with ?
  2. The tag.to_string() and naming::split_tag() unwraps should be handled with proper error propagation

Here's the safer approach:

let tag = model_reader.dojo_name().call().await?;
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!("Failed to split tag: {}", e)))?;

This matches the error handling patterns seen in the codebase, particularly in world.rs where tag parsing errors are properly propagated using ModelError::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 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();
+ let tag = model_reader.dojo_name().call().await?;
+ let tag_str = tag.to_string().map_err(|e| ModelError::TagError(format!("Invalid tag string: {}", e)))?;
+ let (namespace, name) = naming::split_tag(&tag_str)
+     .ok_or_else(|| ModelError::TagError("Failed to split tag".to_string()))?;

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:

#!/bin/bash
# Check if there are any other unwrap calls in tag parsing
rg "unwrap\(\)" --type rust

Length of output: 187299


Script:

#!/bin/bash
# Check how tag parsing is handled in other places
rg "split_tag|naming::" --type rust -A 5

Length of output: 40858


Script:

#!/bin/bash
# Check for ModelError and tag error handling
rg "ModelError|TagError" --type rust -A 5

Length of output: 10819

Copy link

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! The unwrap is indeed unsafe here, sensei!

Looking at the codebase, I found that proper error handling for split_tag exists in other places, particularly in world.rs where it's handled with map_err(|e| ModelError::TagError(e.to_string())). The current unwrap in model.rs bypasses this error handling pattern.

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:

  • world.rs properly handles tag parsing errors with ModelError::TagError
  • TagError exists specifically for handling these cases
  • Multiple test cases in naming.rs show that split_tag can fail
🔗 Analysis chain

Ohayo! 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 split_tag and to_string in the codebase to verify the safety concern.

🏁 Scripts executed

The 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
Expand Down
8 changes: 8 additions & 0 deletions crates/dojo/world/src/contracts/world.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::result::Result;

use starknet::providers::Provider;
use starknet_crypto::Felt;

pub use super::abigen::world::{
ContractRegistered, ContractUpgraded, Event as WorldEvent, ModelRegistered, WorldContract,
Expand Down Expand Up @@ -33,4 +34,11 @@ where
) -> Result<ModelRPCReader<'_, P>, ModelError> {
ModelRPCReader::new(namespace, name, self).await
}

pub async fn model_reader_with_selector(
&self,
selector: Felt,
) -> Result<ModelRPCReader<'_, P>, ModelError> {
ModelRPCReader::new_with_selector(selector, self).await
}
}
2 changes: 2 additions & 0 deletions crates/torii/core/src/processors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ pub mod store_set_record;
pub mod store_transaction;
pub mod store_update_member;
pub mod store_update_record;
pub mod upgrade_model;
pub mod upgrade_event;

const MODEL_INDEX: usize = 0;
const ENTITY_ID_INDEX: usize = 1;
Expand Down
7 changes: 4 additions & 3 deletions crates/torii/core/src/processors/register_event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ where
P: Provider + Send + Sync + std::fmt::Debug,
{
fn event_key(&self) -> String {
"EventRegistered".to_string()
"EventUpgraded".to_string()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ohayo! Critical mismatch between event key and event handling, sensei!

There appears to be an inconsistency in the event handling logic:

  • The event_key() returns "EventUpgraded"
  • But the process() method still matches against WorldEvent::EventRegistered

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

Copy link

Choose a reason for hiding this comment

The 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:

  • crates/dojo/world/src/remote/events_to_remote.rs: Used in event handling and selector generation
  • crates/dojo/world/src/contracts/abigen/world.rs: Used in struct definitions, serialization, and event handling
  • bin/sozo/src/commands/events.rs: Used in event processing
  • crates/torii/core/src/processors/register_event.rs: Used in pattern matching

This change should either:

  1. Keep the original "EventRegistered" event key, or
  2. Create a new event type instead of replacing the existing one
🔗 Analysis chain

Ohayo! Verify event key change impact.

The event key change from "EventRegistered" to "EventUpgraded" needs careful verification to ensure all consumers handle this correctly.

🏁 Scripts executed

The 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
Expand Down Expand Up @@ -71,7 +71,7 @@ where
target: LOG_TARGET,
namespace = %namespace,
name = %name,
"Registered event."
"Upgraded event."
Copy link

Choose a reason for hiding this comment

The 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:

  • crates/torii/core/src/processors/upgrade_event.rs: Still using "Registered event" and "Registered model content"
  • crates/torii/core/src/processors/register_model.rs: Contains "Registered model" and "Registered model content"

These messages should be updated to use "Upgraded" terminology to maintain consistency with the changes in this PR.

🔗 Analysis chain

Verify 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 executed

The 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!(
Expand All @@ -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(
Expand All @@ -95,6 +95,7 @@ where
packed_size,
unpacked_size,
block_timestamp,
true,
)
.await?;

Expand Down
1 change: 1 addition & 0 deletions crates/torii/core/src/processors/register_model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ where
packed_size,
unpacked_size,
block_timestamp,
false,
)
.await?;

Expand Down
102 changes: 102 additions & 0 deletions crates/torii/core/src/processors/upgrade_event.rs
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";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Inconsistent LOG_TARGET module name

Ohayo, sensei! Noticed that the LOG_TARGET constant references register_event:

pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_event";

Since this module is upgrade_event.rs, consider updating the LOG_TARGET to reflect the correct module name for consistency.

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;
Copy link

Choose a reason for hiding this comment

The 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 RegisterEventProcessor and the LOG_TARGET are named for "register events," but this processor handles "EventUpgraded" events, and the file is named upgrade_event.rs. For better clarity and consistency, consider renaming them to reflect that they're processing upgrade events.

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

‼️ 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.

Suggested change
pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_event";
#[derive(Default, Debug)]
pub struct RegisterEventProcessor;
pub(crate) const LOG_TARGET: &str = "torii_core::processors::upgrade_event";
#[derive(Default, Debug)]
pub struct UpgradeEventProcessor;


#[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)
)
}) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ohayo sensei! Avoid using panic! for error handling

Using panic! may cause the application to crash if the event is malformed. It's better to handle the error gracefully and return an error.

Consider replacing the panic! with an error return:

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.

Committable suggestion skipped: line range outside the PR's diff.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid panicking on event parsing failure

Ohayo, sensei! Using panic!() when parsing the event may not be safe, as it can crash the application if an unexpected event is received. Consider handling the error gracefully by returning a proper error.

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.

Committable suggestion skipped: line range outside the PR's diff.

WorldEvent::EventUpgraded(e) => e,
_ => {
unreachable!()
}
Comment on lines +52 to +54
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle unexpected event types explicitly

Ohayo, sensei! Instead of using unreachable!(), return an error when an unexpected event type is encountered. This improves code robustness and makes debugging easier.

Apply this diff:

-_ => {
-    unreachable!()
-}
+_ => {
+    return Err(anyhow!("Unexpected event type received in UpgradeEventProcessor."));
+}

Committable suggestion skipped: line range outside the PR's diff.

};

// 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,
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Ohayo sensei! The is_upgrade flag should indeed be true

Looking at the codebase, I found that:

  • upgrade_model.rs sets is_upgrade to true for model upgrades
  • register_event.rs also sets is_upgrade to true for registrations
  • Only register_model.rs and the file in question set it to false

Since this processor handles upgrade events, it should align with upgrade_model.rs and set is_upgrade to true to maintain consistency in upgrade handling.

🔗 Analysis chain

Ohayo sensei! Verify the is_upgrade flag value

When registering the model in the database, the is_upgrade flag is set to false. Since this processor handles upgrade events, should this flag be set to true?

Run the following script to check how register_model is called elsewhere:

This will help confirm whether the is_upgrade flag should be true in this context.

🏁 Scripts executed

The 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?;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Set is_upgrade parameter to true

Ohayo, sensei! When registering a model during an upgrade event, the is_upgrade parameter should be true to reflect the nature of the event.

Apply this diff:

            block_timestamp,
-            false,
+            true,
        )
📝 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.

Suggested change
db.register_model(
&namespace,
schema,
layout,
event.class_hash.into(),
event.address.into(),
packed_size,
unpacked_size,
block_timestamp,
false,
)
.await?;
db.register_model(
&namespace,
schema,
layout,
event.class_hash.into(),
event.address.into(),
packed_size,
unpacked_size,
block_timestamp,
true,
)
.await?;


Ok(())
}
}
98 changes: 98 additions & 0 deletions crates/torii/core/src/processors/upgrade_model.rs
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";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Inconsistent LOG_TARGET value with module name

Ohayo, sensei! The LOG_TARGET is set to "torii_core::processors::register_model", but this processor handles model upgrades. Consider updating it to "torii_core::processors::upgrade_model" for consistency.


#[derive(Default, Debug)]
pub struct RegisterModelProcessor;
Copy link

Choose a reason for hiding this comment

The 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 RegisterModelProcessor, but it processes model upgrades. Renaming it to UpgradeModelProcessor would more accurately represent its purpose.


#[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)
)
}) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid panicking; handle event parsing errors gracefully

Ohayo, sensei! The current implementation panics if WorldEvent::try_from(event) fails to parse the event, which could lead to unexpected application crashes. It's better to handle this error gracefully by returning an appropriate error. Consider modifying the code as follows:

-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.

Committable suggestion skipped: line range outside the PR's diff.

WorldEvent::ModelUpgraded(e) => e,
_ => {
unreachable!()
}
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid panic! in async context; return error instead

Ohayo, sensei! Using panic! within an async function can lead to unexpected behavior and might crash the entire async runtime. It's better to handle errors gracefully by returning them. This ensures that the application remains robust and errors are propagated correctly.

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

‼️ 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.

Suggested change
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
)));
}
};


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(())
}
}
Loading
Loading