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 37 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
17 changes: 13 additions & 4 deletions bin/torii/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ use torii_core::executor::Executor;
use torii_core::processors::store_transaction::StoreTransactionProcessor;
use torii_core::processors::EventProcessorConfig;
use torii_core::simple_broker::SimpleBroker;
use torii_core::sql::cache::ModelCache;
use torii_core::sql::Sql;
use torii_core::types::{Contract, ContractType, Model};
use torii_server::proxy::Proxy;
Expand Down Expand Up @@ -243,7 +244,9 @@ async fn main() -> anyhow::Result<()> {
executor.run().await.unwrap();
});

let db = Sql::new(pool.clone(), sender.clone(), &args.indexing.contracts).await?;
let model_cache = Arc::new(ModelCache::new(pool.clone()));
let db = Sql::new(pool.clone(), sender.clone(), &args.indexing.contracts, model_cache.clone())
.await?;

let processors = Processors {
transaction: vec![Box::new(StoreTransactionProcessor)],
Expand Down Expand Up @@ -283,9 +286,15 @@ async fn main() -> anyhow::Result<()> {
);

let shutdown_rx = shutdown_tx.subscribe();
let (grpc_addr, grpc_server) =
torii_grpc::server::new(shutdown_rx, &pool, block_rx, world_address, Arc::clone(&provider))
.await?;
let (grpc_addr, grpc_server) = torii_grpc::server::new(
shutdown_rx,
&pool,
block_rx,
world_address,
Arc::clone(&provider),
model_cache,
)
.await?;

let mut libp2p_relay_server = torii_relay::server::Relay::new(
db,
Expand Down
55 changes: 19 additions & 36 deletions crates/dojo/types/src/primitive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,46 +190,29 @@ impl Primitive {
}
}

pub fn to_sql_value(&self) -> Result<String, PrimitiveError> {
let value = self.serialize()?;

if value.is_empty() {
return Err(PrimitiveError::MissingFieldElement);
}

pub fn to_sql_value(&self) -> String {
match self {
// Integers
Primitive::I8(_) => Ok(format!("{}", try_from_felt::<i8>(value[0])?)),
Primitive::I16(_) => Ok(format!("{}", try_from_felt::<i16>(value[0])?)),
Primitive::I32(_) => Ok(format!("{}", try_from_felt::<i32>(value[0])?)),
Primitive::I64(_) => Ok(format!("{}", try_from_felt::<i64>(value[0])?)),
Primitive::I8(i8) => format!("{}", i8.unwrap_or_default()),
Primitive::I16(i16) => format!("{}", i16.unwrap_or_default()),
Primitive::I32(i32) => format!("{}", i32.unwrap_or_default()),
Primitive::I64(i64) => format!("{}", i64.unwrap_or_default()),

Primitive::U8(_)
| Primitive::U16(_)
| Primitive::U32(_)
| Primitive::USize(_)
| Primitive::Bool(_) => Ok(format!("{}", value[0])),
Primitive::U8(u8) => format!("{}", u8.unwrap_or_default()),
Primitive::U16(u16) => format!("{}", u16.unwrap_or_default()),
Primitive::U32(u32) => format!("{}", u32.unwrap_or_default()),
Primitive::USize(u32) => format!("{}", u32.unwrap_or_default()),
Primitive::Bool(bool) => format!("{}", bool.unwrap_or_default() as i32),

// Hex string
Primitive::I128(_) => Ok(format!("{:#064x}", try_from_felt::<i128>(value[0])?)),
Primitive::ContractAddress(_)
| Primitive::ClassHash(_)
| Primitive::Felt252(_)
| Primitive::U128(_)
| Primitive::U64(_) => Ok(format!("{:#064x}", value[0])),

Primitive::U256(_) => {
if value.len() < 2 {
Err(PrimitiveError::NotEnoughFieldElements)
} else {
let mut buffer = [0u8; 32];
let value0_bytes = value[0].to_bytes_be();
let value1_bytes = value[1].to_bytes_be();
buffer[16..].copy_from_slice(&value0_bytes[16..]);
buffer[..16].copy_from_slice(&value1_bytes[16..]);
Ok(format!("0x{}", hex::encode(buffer)))
}
}
Primitive::I128(i128) => format!("{:#064x}", i128.unwrap_or_default()),
Primitive::ContractAddress(felt) => format!("{:#064x}", felt.unwrap_or_default()),
Primitive::ClassHash(felt) => format!("{:#064x}", felt.unwrap_or_default()),
Primitive::Felt252(felt) => format!("{:#064x}", felt.unwrap_or_default()),
Primitive::U128(u128) => format!("{:#064x}", u128.unwrap_or_default()),
Primitive::U64(u64) => format!("{:#064x}", u64.unwrap_or_default()),

Primitive::U256(u256) => format!("0x{:064x}", u256.unwrap_or_default()),
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! Standardize hex formatting across all types

The hex formatting is inconsistent:

  • Most types use {:#064x} format
  • U256 uses 0x{:064x} format

This inconsistency could cause issues when parsing these values.

Apply this change:

-            Primitive::U256(u256) => format!("0x{:064x}", u256.unwrap_or_default()),
+            Primitive::U256(u256) => format!("{:#064x}", u256.unwrap_or_default()),
📝 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
Primitive::I128(i128) => format!("{:#064x}", i128.unwrap_or_default()),
Primitive::ContractAddress(felt) => format!("{:#064x}", felt.unwrap_or_default()),
Primitive::ClassHash(felt) => format!("{:#064x}", felt.unwrap_or_default()),
Primitive::Felt252(felt) => format!("{:#064x}", felt.unwrap_or_default()),
Primitive::U128(u128) => format!("{:#064x}", u128.unwrap_or_default()),
Primitive::U64(u64) => format!("{:#064x}", u64.unwrap_or_default()),
Primitive::U256(u256) => format!("0x{:064x}", u256.unwrap_or_default()),
Primitive::I128(i128) => format!("{:#064x}", i128.unwrap_or_default()),
Primitive::ContractAddress(felt) => format!("{:#064x}", felt.unwrap_or_default()),
Primitive::ClassHash(felt) => format!("{:#064x}", felt.unwrap_or_default()),
Primitive::Felt252(felt) => format!("{:#064x}", felt.unwrap_or_default()),
Primitive::U128(u128) => format!("{:#064x}", u128.unwrap_or_default()),
Primitive::U64(u64) => format!("{:#064x}", u64.unwrap_or_default()),
Primitive::U256(u256) => format!("{:#064x}", u256.unwrap_or_default()),

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! Critical: Avoid silent defaults in database operations

The use of unwrap_or_default() in to_sql_value could silently store default values in the database when None is encountered, potentially leading to data integrity issues. Consider handling None values explicitly.

Consider this approach:

-    pub fn to_sql_value(&self) -> String {
+    pub fn to_sql_value(&self) -> Result<String, PrimitiveError> {
         match self {
             // Integers
-            Primitive::I8(i8) => format!("{}", i8.unwrap_or_default()),
+            Primitive::I8(i8) => i8.map(|v| format!("{}", v))
+                .ok_or(PrimitiveError::MissingFieldElement),
             // Apply similar changes to other variants...
         }
     }

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

}
}

Expand Down Expand Up @@ -436,7 +419,7 @@ mod tests {
let primitive = Primitive::U256(Some(U256::from_be_hex(
"aaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbccccccccccccccccdddddddddddddddd",
)));
let sql_value = primitive.to_sql_value().unwrap();
let sql_value = primitive.to_sql_value();
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! Add error case tests

The test only covers the happy path with Some values. We should also test error cases with None values.

Add these test cases:

#[test]
fn test_u256_none() {
    let primitive = Primitive::U256(None);
    assert!(matches!(
        primitive.serialize(),
        Err(PrimitiveError::MissingFieldElement)
    ));
}

let serialized = primitive.serialize().unwrap();

let mut deserialized = primitive;
Expand Down
161 changes: 159 additions & 2 deletions crates/dojo/types/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,11 @@ impl Ty {
}

pub fn deserialize(&mut self, felts: &mut Vec<Felt>) -> Result<(), PrimitiveError> {
if felts.is_empty() {
// return early if there are no felts to deserialize
return Ok(());
}

match self {
Ty::Primitive(c) => {
c.deserialize(felts)?;
Expand Down Expand Up @@ -224,6 +229,85 @@ impl Ty {
}
Ok(())
}

/// Returns a new Ty containing only the differences between self and other
pub fn diff(&self, other: &Ty) -> Option<Ty> {
match (self, other) {
(Ty::Struct(s1), Ty::Struct(s2)) => {
// Find members that exist in s1 but not in s2, or are different
let diff_children: Vec<Member> = s1
.children
.iter()
.filter(|m1| {
s2.children
.iter()
.find(|m2| m2.name == m1.name)
.map_or(true, |m2| *m1 != m2)
})
.cloned()
.collect();

if diff_children.is_empty() {
None
} else {
Some(Ty::Struct(Struct { name: s1.name.clone(), children: diff_children }))
}
}
(Ty::Enum(e1), Ty::Enum(e2)) => {
// Find options that exist in e1 but not in e2, or are different
let diff_options: Vec<EnumOption> = e1
.options
.iter()
.filter(|o1| {
e2.options.iter().find(|o2| o2.name == o1.name).map_or(true, |o2| *o1 != o2)
})
.cloned()
.collect();

if diff_options.is_empty() {
None
} else {
Some(Ty::Enum(Enum {
name: e1.name.clone(),
option: e1.option,
options: diff_options,
}))
}
}
(Ty::Array(a1), Ty::Array(a2)) => {
if a1 == a2 {
None
} else {
Some(Ty::Array(a1.clone()))
}
}
(Ty::Tuple(t1), Ty::Tuple(t2)) => {
if t1 == t2 {
None
} else {
Some(Ty::Tuple(t1.clone()))
}
}
(Ty::ByteArray(b1), Ty::ByteArray(b2)) => {
if b1 == b2 {
None
} else {
Some(Ty::ByteArray(b1.clone()))
}
}
(Ty::Primitive(p1), Ty::Primitive(p2)) => {
if p1 == p2 {
None
} else {
Some(Ty::Primitive(*p1))
}
}
// Different types entirely - we cannot diff them
_ => {
panic!("Type mismatch between self {:?} and other {:?}", self.name(), other.name())
}
}
}
}

#[derive(Debug)]
Expand Down Expand Up @@ -351,8 +435,8 @@ impl Enum {
}
}

pub fn to_sql_value(&self) -> Result<String, EnumError> {
self.option().map(|option| option.name.clone())
pub fn to_sql_value(&self) -> String {
self.option().unwrap_or(&self.options[0]).name.clone()
Comment on lines +438 to +439
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential panics in to_sql_value.

Ohayo! The unwrap_or usage could panic if the enum option is invalid.

Consider this safer implementation:

- pub fn to_sql_value(&self) -> String {
-     self.option().unwrap_or(&self.options[0]).name.clone()
+ pub fn to_sql_value(&self) -> Result<String, EnumError> {
+     Ok(self.option().unwrap_or_else(|_| &self.options[0]).name.clone())

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

}
}

Expand Down Expand Up @@ -597,4 +681,77 @@ mod tests {
assert_eq!(format_member(&member), expected);
}
}

#[test]
fn test_ty_diff() {
// Test struct diff
let struct1 = Ty::Struct(Struct {
name: "TestStruct".to_string(),
children: vec![
Member {
name: "field1".to_string(),
ty: Ty::Primitive(Primitive::U32(None)),
key: false,
},
Member {
name: "field2".to_string(),
ty: Ty::Primitive(Primitive::U32(None)),
key: false,
},
Member {
name: "field3".to_string(),
ty: Ty::Primitive(Primitive::U32(None)),
key: false,
},
],
});

let struct2 = Ty::Struct(Struct {
name: "TestStruct".to_string(),
children: vec![Member {
name: "field1".to_string(),
ty: Ty::Primitive(Primitive::U32(None)),
key: false,
}],
});

// Should show only field2 and field3 as differences
let diff = struct1.diff(&struct2).unwrap();
if let Ty::Struct(s) = diff {
assert_eq!(s.children.len(), 2);
assert_eq!(s.children[0].name, "field2");
assert_eq!(s.children[1].name, "field3");
} else {
panic!("Expected Struct diff");
}

// Test enum diff
let enum1 = Ty::Enum(Enum {
name: "TestEnum".to_string(),
option: None,
options: vec![
EnumOption { name: "Option1".to_string(), ty: Ty::Tuple(vec![]) },
EnumOption { name: "Option2".to_string(), ty: Ty::Tuple(vec![]) },
],
});

let enum2 = Ty::Enum(Enum {
name: "TestEnum".to_string(),
option: None,
options: vec![EnumOption { name: "Option1".to_string(), ty: Ty::Tuple(vec![]) }],
});

// Should show only Option2 as difference
let diff = enum1.diff(&enum2).unwrap();
if let Ty::Enum(e) = diff {
assert_eq!(e.options.len(), 1);
assert_eq!(e.options[0].name, "Option2");
} else {
panic!("Expected Enum diff");
}

// Test no differences
let same_struct = struct2.diff(&struct2);
assert!(same_struct.is_none());
}
Larkooo marked this conversation as resolved.
Show resolved Hide resolved
}
18 changes: 14 additions & 4 deletions crates/dojo/world/src/contracts/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use cainome::cairo_serde::{CairoSerde as _, ContractAddress, Error as CainomeErr
use dojo_types::packing::{PackingError, ParseError};
use dojo_types::primitive::{Primitive, PrimitiveError};
use dojo_types::schema::{Enum, EnumOption, Member, Struct, Ty};
use starknet::core::types::Felt;
use starknet::core::types::{BlockId, Felt};
use starknet::core::utils::{
cairo_short_string_to_felt, parse_cairo_short_string, CairoShortStringToFeltError,
NonAsciiNameError, ParseCairoShortStringError,
Expand Down Expand Up @@ -86,13 +86,22 @@ where
namespace: &str,
name: &str,
world: &'a WorldContractReader<P>,
) -> Result<ModelRPCReader<'a, P>, ModelError> {
Self::new_with_block(namespace, name, world, world.block_id).await
}

pub async fn new_with_block(
namespace: &str,
name: &str,
world: &'a WorldContractReader<P>,
block_id: BlockId,
) -> Result<ModelRPCReader<'a, P>, ModelError> {
let model_selector = naming::compute_selector_from_names(namespace, name);

// 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(&model_selector).block_id(world.block_id).call().await? {
match world.resource(&model_selector).block_id(block_id).call().await? {
abigen::world::Resource::Model((address, hash)) => (address, hash),
abigen::world::Resource::Event((address, hash)) => (address, hash),
_ => return Err(ModelError::ModelNotFound),
Expand All @@ -104,7 +113,8 @@ where
return Err(ModelError::ModelNotFound);
}

let model_reader = ModelContractReader::new(contract_address.into(), world.provider());
let mut model_reader = ModelContractReader::new(contract_address.into(), world.provider());
model_reader.set_block(block_id);

Ok(Self {
namespace: namespace.into(),
Expand Down Expand Up @@ -176,7 +186,7 @@ where
parse_schema(&abigen::model::Ty::Struct(res)).map_err(ModelError::Parse)
}

// For non fixed layouts, packed and unpacked sizes are None.
// For non fixed layouts, packed and unpacked sizes are None.
// Therefore we return 0 in this case.
async fn packed_size(&self) -> Result<u32, ModelError> {
Ok(self.model_reader.packed_size().call().await?.unwrap_or(0))
Expand Down
10 changes: 10 additions & 0 deletions crates/dojo/world/src/contracts/world.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::result::Result;

use starknet::core::types::BlockId;
use starknet::providers::Provider;

pub use super::abigen::world::{
Expand Down Expand Up @@ -33,4 +34,13 @@ where
) -> Result<ModelRPCReader<'_, P>, ModelError> {
ModelRPCReader::new(namespace, name, self).await
}

pub async fn model_reader_with_block(
&self,
namespace: &str,
name: &str,
block_id: BlockId,
) -> Result<ModelRPCReader<'_, P>, ModelError> {
ModelRPCReader::new_with_block(namespace, name, self, block_id).await
}
}
2 changes: 1 addition & 1 deletion crates/sozo/ops/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ fn format_primitive(
let mut _p = *p;
let _ = _p.deserialize(values);

format!("{}{}", _start_indent(level, start_indent), _p.to_sql_value().unwrap())
format!("{}{}", _start_indent(level, start_indent), _p.to_sql_value())
}

fn format_byte_array(values: &mut Vec<Felt>, level: usize, start_indent: bool) -> String {
Expand Down
4 changes: 4 additions & 0 deletions crates/torii/core/src/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ use crate::processors::store_del_record::StoreDelRecordProcessor;
use crate::processors::store_set_record::StoreSetRecordProcessor;
use crate::processors::store_update_member::StoreUpdateMemberProcessor;
use crate::processors::store_update_record::StoreUpdateRecordProcessor;
use crate::processors::upgrade_event::UpgradeEventProcessor;
use crate::processors::upgrade_model::UpgradeModelProcessor;
use crate::processors::{
BlockProcessor, EventProcessor, EventProcessorConfig, TransactionProcessor,
};
Expand Down Expand Up @@ -76,6 +78,8 @@ impl<P: Provider + Send + Sync + std::fmt::Debug + 'static> Processors<P> {
vec![
Box::new(RegisterModelProcessor) as Box<dyn EventProcessor<P>>,
Box::new(RegisterEventProcessor) as Box<dyn EventProcessor<P>>,
Box::new(UpgradeModelProcessor) as Box<dyn EventProcessor<P>>,
Box::new(UpgradeEventProcessor) as Box<dyn EventProcessor<P>>,
Box::new(StoreSetRecordProcessor),
Box::new(StoreDelRecordProcessor),
Box::new(StoreUpdateRecordProcessor),
Expand Down
Loading
Loading