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(lang): add the ability to get a WorldStorage from a namespace hash #2686

Closed
wants to merge 12 commits into from
6 changes: 3 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,13 @@ katana-trie = { path = "crates/katana/trie" }
# torii
torii-cli = { path = "crates/torii/cli" }
torii-client = { path = "crates/torii/client" }
torii-indexer = { path = "crates/torii/indexer" }
torii-sqlite = { path = "crates/torii/sqlite" }
torii-graphql = { path = "crates/torii/graphql" }
torii-grpc = { path = "crates/torii/grpc" }
torii-indexer = { path = "crates/torii/indexer" }
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! Missing workspace members for torii dependencies

The directories exist, but crates/torii/indexer and crates/torii/sqlite are not listed in the workspace members. These need to be added to the members array in Cargo.toml for the workspace dependencies to work properly.

  • Add "crates/torii/indexer" to the workspace members
  • Add "crates/torii/sqlite" to the workspace members
🔗 Analysis chain

Ohayo sensei! Verify workspace member entries for new torii dependencies.

The added dependencies look well-structured, but let's ensure their corresponding directories are properly listed in the workspace members.

Let's run this check:

Also applies to: 127-128

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the crate directories exist and are listed in workspace members

# Check if directories exist
echo "Checking if directories exist:"
for dir in "crates/torii/indexer" "crates/torii/server" "crates/torii/sqlite"; do
    if [ -d "$dir" ]; then
        echo "✓ Directory exists: $dir"
    else
        echo "✗ Missing directory: $dir"
    fi
done

# Check if they're listed in workspace members
echo -e "\nChecking workspace members entries:"
grep -A 1 "members = \[" Cargo.toml | grep -E "crates/torii/(indexer|server|sqlite)"

Length of output: 853


Script:

#!/bin/bash
# Let's see the full Cargo.toml content to understand the workspace members format
cat Cargo.toml

Length of output: 9831

torii-relay = { path = "crates/torii/libp2p" }
torii-server = { path = "crates/torii/server" }
torii-runner = { path = "crates/torii/runner" }
torii-server = { path = "crates/torii/server" }
torii-sqlite = { path = "crates/torii/sqlite" }
torii-typed-data = { path = "crates/torii/typed-data" }

# saya
Expand Down
1 change: 1 addition & 0 deletions crates/dojo/core-cairo-test/src/lib.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ mod tests {

mod expanded {
pub(crate) mod selector_attack;
mod poseidon_hash_string;
}

mod helpers {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
use core::poseidon::poseidon_hash_span;


glihm marked this conversation as resolved.
Show resolved Hide resolved
#[test]
fn test_poseidon_hash_string() {
let bytes: ByteArray = "foo";
let hash = poseidon_hash_string!("foo");
let mut array = array![];
bytes.serialize(ref array);
let computed = poseidon_hash_span(array.span());
assert_eq!(computed, hash);
}
#[test]
fn test_poseidon_hash_string_empty() {
let bytes: ByteArray = "";
let hash = poseidon_hash_string!("");
let mut array = array![];
bytes.serialize(ref array);
let computed = poseidon_hash_span(array.span());
assert_eq!(computed, hash);
}

#[test]
fn test_poseidon_hash_string_31() {
let bytes: ByteArray = "0123456789012345678901234567890";
let hash = poseidon_hash_string!("0123456789012345678901234567890");
let mut array = array![];
bytes.serialize(ref array);
let computed = poseidon_hash_span(array.span());
assert_eq!(computed, hash);
}

#[test]
fn test_poseidon_hash_string_long() {
let bytes: ByteArray = "0123456789012345678901234567890foo";
let hash = poseidon_hash_string!("0123456789012345678901234567890foo");
let mut array = array![];
bytes.serialize(ref array);
let computed = poseidon_hash_span(array.span());
assert_eq!(computed, hash);
}

#[test]
fn test_poseidon_hash_string_ne() {
let bytes: ByteArray = "foo";
let hash = poseidon_hash_string!("bar");
let mut array = array![];
bytes.serialize(ref array);
let computed = poseidon_hash_span(array.span());
assert_ne!(computed, hash);
}
Comment on lines +44 to +51
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Missing #[test] attribute and consider additional negative cases!

  1. The test won't be executed without the test attribute.
  2. Consider adding more negative tests:
    • Similar strings (e.g., "foo" vs "fooo")
    • Case variations (e.g., "foo" vs "FOO")
    • Different string lengths with same content

Apply this diff to fix the attribute:

+#[test]
 fn test_poseidon_hash_string_ne() {
📝 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
fn test_poseidon_hash_string_ne() {
let bytes: ByteArray = "foo";
let hash = poseidon_hash_string!("bar");
let mut array = array![];
bytes.serialize(ref array);
let computed = poseidon_hash_span(array.span());
assert_ne!(computed, hash);
}
#[test]
fn test_poseidon_hash_string_ne() {
let bytes: ByteArray = "foo";
let hash = poseidon_hash_string!("bar");
let mut array = array![];
bytes.serialize(ref array);
let computed = poseidon_hash_span(array.span());
assert_ne!(computed, hash);
}


33 changes: 23 additions & 10 deletions crates/dojo/core/src/world/storage.cairo
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! A simple storage abstraction for the world's storage.

use core::panic_with_felt252;
use core::poseidon::poseidon_hash_span;
use dojo::world::{IWorldDispatcher, IWorldDispatcherTrait, Resource};
use dojo::model::{Model, ModelIndex, ModelValueKey, ModelValue, ModelStorage, ModelPtr};
use dojo::event::{Event, EventStorage};
Expand Down Expand Up @@ -32,22 +33,34 @@ pub impl WorldStorageInternalImpl of WorldStorageTrait {
WorldStorage { dispatcher: world, namespace_hash }
}

fn new_from_hash(world: IWorldDispatcher, namespace_hash: felt252) -> WorldStorage {
WorldStorage { dispatcher: world, namespace_hash }
}

Comment on lines +36 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we add this to save the Poseidon hashes on the serialized byte array, we should also add one for the dns. Cairo doesn't support function overloading.

Since computing the dojo selector requires three steps:

  1. Hashing the namespace (saved by the new_from_hash).
  2. Hashing the name.
  3. Hashing them together.
    Having something like:
let (ca, ch) = world.dns_from_hash/selector(selector_from_tag!("eternum-map_system")).unwrap();

Will be the most efficient way to query a resource, with no Poseidon hash at all.

fn set_namespace(ref self: WorldStorage, namespace: @ByteArray) {
self.namespace_hash = dojo::utils::bytearray_hash(namespace);
}


fn dns(self: @WorldStorage, contract_name: @ByteArray) -> Option<(ContractAddress, ClassHash)> {
match (*self.dispatcher)
.resource(
dojo::utils::selector_from_namespace_and_name(*self.namespace_hash, contract_name),
) {
Self::dns_from_hash(self, dojo::utils::bytearray_hash(contract_name))
}

fn dns_from_hash(
self: @WorldStorage, contract_name_hash: felt252,
) -> Option<(ContractAddress, ClassHash)> {
Self::dns_from_selector(
self, poseidon_hash_span([*self.namespace_hash, contract_name_hash].span()),
)
}

fn dns_from_selector(
self: @WorldStorage, selector: felt252,
) -> Option<(ContractAddress, ClassHash)> {
match (*self.dispatcher).resource(selector) {
Resource::Contract((
contract_address, _,
)) => {
let class_hash = starknet::syscalls::get_class_hash_at_syscall(contract_address)
.expect('Failed to get class hash');
Option::Some((contract_address, class_hash))
},
contract_address, class_hash,
)) => { Option::Some((contract_address, class_hash.try_into().unwrap())) },
_ => Option::None,
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ pub mod $name$ {
fn world(self: @ContractState, namespace: @ByteArray) -> dojo::world::storage::WorldStorage {
dojo::world::WorldStorageTrait::new(self.world_provider.world_dispatcher(), namespace)
}

fn world_from_hash(self: @ContractState, namespace_hash: felt252) -> dojo::world::storage::WorldStorage {
dojo::world::WorldStorageTrait::new_from_hash(self.world_provider.world_dispatcher(), namespace_hash)
}
}

$body$
Expand Down
3 changes: 2 additions & 1 deletion crates/dojo/lang/src/cairo_plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use super::attribute_macros::{
DojoContract, DojoEvent, DojoModel, DOJO_CONTRACT_ATTR, DOJO_EVENT_ATTR, DOJO_MODEL_ATTR,
};
use super::derive_macros::{dojo_derive_all, DOJO_INTROSPECT_DERIVE, DOJO_PACKED_DERIVE};
use super::inline_macros::SelectorFromTagMacro;
use super::inline_macros::{PoseidonHashStringMacro, SelectorFromTagMacro};

// #[cfg(test)]
// #[path = "plugin_test.rs"]
Expand All @@ -26,6 +26,7 @@ pub fn dojo_plugin_suite() -> PluginSuite {
let mut suite = PluginSuite::default();

suite.add_plugin::<BuiltinDojoPlugin>().add_inline_macro_plugin::<SelectorFromTagMacro>();
suite.add_plugin::<BuiltinDojoPlugin>().add_inline_macro_plugin::<PoseidonHashStringMacro>();

suite
}
Expand Down
2 changes: 2 additions & 0 deletions crates/dojo/lang/src/inline_macros/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ pub mod delete;
pub mod emit;
pub mod get;
pub mod get_models_test_class_hashes;
pub mod poseidon_hash_string;
pub mod selector_from_tag;
pub mod set;
pub mod spawn_test_world;
Expand All @@ -21,6 +22,7 @@ pub use delete::DeleteMacro;
pub use emit::EmitMacro;
pub use get::GetMacro;
pub use get_models_test_class_hashes::GetModelsTestClassHashes;
pub use poseidon_hash_string::PoseidonHashStringMacro;
pub use selector_from_tag::SelectorFromTagMacro;
pub use set::SetMacro;
pub use spawn_test_world::SpawnTestWorld;
Expand Down
62 changes: 62 additions & 0 deletions crates/dojo/lang/src/inline_macros/poseidon_hash_string.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
use cairo_lang_defs::patcher::PatchBuilder;
use cairo_lang_defs::plugin::{
InlineMacroExprPlugin, InlinePluginResult, MacroPluginMetadata, NamedPlugin, PluginDiagnostic,
PluginGeneratedFile,
};
use cairo_lang_defs::plugin_utils::unsupported_bracket_diagnostic;
use cairo_lang_diagnostics::Severity;
use cairo_lang_syntax::node::{ast, TypedStablePtr, TypedSyntaxNode};
use dojo_types::naming;

#[derive(Debug, Default)]
pub struct PoseidonHashStringMacro;

impl NamedPlugin for PoseidonHashStringMacro {
const NAME: &'static str = "poseidon_hash_string";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const NAME: &'static str = "poseidon_hash_string";
const NAME: &'static str = "poseidon_hash_string";

Wondering if poseidon_hash_bytearray would not be more consistent with the dojo utils functions already using bytearray instead of string.

}

impl InlineMacroExprPlugin for PoseidonHashStringMacro {
fn generate_code(
&self,
db: &dyn cairo_lang_syntax::node::db::SyntaxGroup,
syntax: &ast::ExprInlineMacro,
_metadata: &MacroPluginMetadata<'_>,
) -> InlinePluginResult {
let ast::WrappedArgList::ParenthesizedArgList(arg_list) = syntax.arguments(db) else {
return unsupported_bracket_diagnostic(db, syntax);
};

let args = arg_list.arguments(db).elements(db);

if args.len() != 1 {
return InlinePluginResult {
code: None,
diagnostics: vec![PluginDiagnostic {
stable_ptr: syntax.stable_ptr().untyped(),
message: "Invalid arguments. Expected \"poseidon_hash_string!(\"tag\")\""
.to_string(),
severity: Severity::Error,
}],
};
}

let tag = &args[0].as_syntax_node().get_text(db).replace('\"', "");
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! Properly handle string literals for the tag.

Using replace('\"', "") may not correctly handle all cases, especially if the tag contains escaped quotes or special characters.

Consider parsing the string literal to accurately extract the tag value, handling any escaped characters appropriately.


let selector = naming::compute_bytearray_hash(tag);

let mut builder = PatchBuilder::new(db, syntax);
builder.add_str(&format!("{:#64x}", selector));

let (code, code_mappings) = builder.build();

InlinePluginResult {
code: Some(PluginGeneratedFile {
name: "poseidon_hash_string_macro".into(),
content: code,
code_mappings,
aux_data: None,
}),
diagnostics: vec![],
}
}
}
2 changes: 2 additions & 0 deletions crates/dojo/lang/src/semantics/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ test_file_test!(
get_models_test_class_hashes: "get_models_test_class_hashes",

spawn_test_world: "spawn_test_world",

poseidon_hash_string: "poseidon_hash_string",
Comment on lines +25 to +26
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! Missing test file for poseidon_hash_string

The test case poseidon_hash_string is added to the test macro but the corresponding test file is missing in crates/dojo/lang/src/semantics/test_data/. All other test files listed in the macro have their corresponding files present in the test directory.

  • Missing test file: crates/dojo/lang/src/semantics/test_data/poseidon_hash_string
  • Other test files like get, set, selector_from_tag etc. are properly present
🔗 Analysis chain

Ohayo sensei! Verify the test file existence and structure.

The new test case poseidon_hash_string has been added to the test suite, but we should verify that the corresponding test file exists at the expected location.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence and structure of the poseidon_hash_string test file

# Check if the test file exists
fd "poseidon_hash_string" "src/plugin/semantics/test_data"

# If found, examine its content for proper test structure
if [ $? -eq 0 ]; then
  rg -A 10 "test|assert" "src/plugin/semantics/test_data/poseidon_hash_string.cairo"
fi

Length of output: 192


Script:

#!/bin/bash
# Let's first find the correct test data directory structure
fd -t d "test_data"

# Then search for the poseidon test file in the correct location
fd "poseidon_hash_string" .

# Let's also check the test file structure in semantics/tests.rs to understand the pattern
rg -A 5 "test_file_test!" "crates/dojo/lang/src/semantics/tests.rs"

Length of output: 586


Script:

#!/bin/bash
# Let's check if the test file exists in the correct location
fd "poseidon_hash_string" "crates/dojo/lang/src/semantics/test_data"

# Let's also check the content of the test macro to understand the full context
cat "crates/dojo/lang/src/semantics/tests.rs"

# And examine the actual test files that exist
ls -la "crates/dojo/lang/src/semantics/test_data" 2>/dev/null || echo "Directory not found"

Length of output: 2474

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
poseidon_hash_string: "poseidon_hash_string",

Can be removed, this file is not used for now and will be deprecated with proc macros.

},
test_semantics
);
Expand Down
19 changes: 18 additions & 1 deletion examples/spawn-and-move/src/actions.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,18 @@ pub mod actions {
fn world_default(self: @ContractState) -> dojo::world::WorldStorage {
self.world(@"ns")
}

/// A gas optimized version of `world_default`, where hash are computed at compile time.
fn world_default_from_hash(self: @ContractState) -> dojo::world::WorldStorage {
self.world_from_hash(poseidon_hash_string!("ns"))
}
}
}

#[cfg(test)]
mod tests {
use dojo::model::{ModelStorage, ModelValueStorage, ModelStorageTest};
use dojo::world::WorldStorageTrait;
use dojo::world::{WorldStorageTrait};
use dojo_cairo_test::{
spawn_test_world, NamespaceDef, TestResource, ContractDefTrait, ContractDef,
WorldStorageTestTrait,
Expand Down Expand Up @@ -323,6 +328,18 @@ mod tests {
assert(new_position.vec.y == initial_position.vec.y, 'position y is wrong');
}

#[test]
#[available_gas(30000000)]
fn test_world_from_hash() {
let ndef = namespace_def();
let mut world = spawn_test_world([ndef].span());
world.sync_perms_and_inits(contract_defs());
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be removed as not used.

Suggested change
world.sync_perms_and_inits(contract_defs());

let hash: felt252 = poseidon_hash_string!("ns");
let storage = dojo::world::WorldStorageTrait::new_from_hash(world.dispatcher, hash);
assert_eq!(storage.namespace_hash, world.namespace_hash);
assert_eq!(storage.dispatcher.contract_address, world.dispatcher.contract_address);
}

#[test]
#[available_gas(30000000)]
#[cfg(feature: 'dungeon')]
Expand Down
Loading