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.

Sign up for GitHub

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: separate the resource registration and permission phase for tes… #2627

Merged
merged 3 commits into from
Nov 5, 2024
Merged

feat: separate the resource registration and permission phase for tes… #2627

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6229eaf
feat: separate the resource registration and permission phase for tes…
glihm Nov 4, 2024
7711390
ci: add simple and core-cairo-test to tests
glihm Nov 4, 2024
8651687
tests: fix world tests
glihm Nov 5, 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
2 changes: 1 addition & 1 deletion crates/dojo/core-cairo-test/src/lib.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ pub use utils::{GasCounter, assert_array, GasCounterTrait};
#[cfg(target: "test")]
pub use world::{
deploy_contract, deploy_with_world_address, spawn_test_world, NamespaceDef, TestResource,
ContractDef, ContractDefTrait
ContractDef, ContractDefTrait, WorldStorageTestTrait,
};

#[cfg(test)]
Expand Down
4 changes: 2 additions & 2 deletions crates/dojo/core-cairo-test/src/tests/helpers.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use dojo::world::{
};
use dojo::model::Model;

use crate::world::{spawn_test_world, NamespaceDef, TestResource, ContractDefTrait};
use crate::world::{spawn_test_world, NamespaceDef, TestResource};

pub const DOJO_NSH: felt252 = 0x309e09669bc1fdc1dd6563a7ef862aa6227c97d099d08cc7b81bad58a7443fa;

Expand Down Expand Up @@ -238,7 +238,7 @@ pub fn deploy_world_and_bar() -> (IWorldDispatcher, IbarDispatcher) {
let namespace_def = NamespaceDef {
namespace: "dojo", resources: [
TestResource::Model(m_Foo::TEST_CLASS_HASH.try_into().unwrap()),
TestResource::Contract(ContractDefTrait::new(bar::TEST_CLASS_HASH, "bar")),
TestResource::Model(bar::TEST_CLASS_HASH.try_into().unwrap()),
].span(),
};

Expand Down
131 changes: 91 additions & 40 deletions crates/dojo/core-cairo-test/src/world.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,45 +2,65 @@ use core::option::OptionTrait;
use core::result::ResultTrait;
use core::traits::{Into, TryInto};

use starknet::{ContractAddress, ClassHash, syscalls::deploy_syscall};
use starknet::{ContractAddress, syscalls::deploy_syscall};

use dojo::world::{world, IWorldDispatcher, IWorldDispatcherTrait, WorldStorageTrait, WorldStorage};

pub type TestClassHash = felt252;

/// In Cairo test runner, all the classes are expected to be declared already.
/// If a contract belong to an other crate, it must be added to the `build-external-contract`,
/// event for testing, since Scarb does not do that automatically anymore.
///
/// The [`TestResource`] enum uses a felt252 to represent the class hash, this avoids
/// having to write `bar::TEST_CLASS_HASH.try_into().unwrap()` in the test file, simply use
/// `bar::TEST_CLASS_HASH`.
#[derive(Drop)]
pub enum TestResource {
Event: ClassHash,
Model: ClassHash,
Contract: ContractDef,
Event: TestClassHash,
Model: TestClassHash,
Contract: TestClassHash,
}

#[derive(Drop)]
pub struct NamespaceDef {
pub namespace: ByteArray,
pub resources: Span<TestResource>,
#[derive(Drop, Copy)]
pub enum ContractDescriptor {
/// Address of the contract.
Address: ContractAddress,
/// Namespace and name of the contract.
Named: (@ByteArray, @ByteArray),
}

#[derive(Drop)]
/// Definition of a contract to register in the world.
///
/// You can use this struct for a dojo contract, but also for an external contract.
/// The only difference is the `init_calldata`, which is only used for dojo contracts.
/// If the `contract` is an external contract (hence an address), then `init_calldata` is ignored.
#[derive(Drop, Copy)]
pub struct ContractDef {
/// Class hash, use `felt252` instead of `ClassHash` as TEST_CLASS_HASH is a `felt252`.
pub class_hash: felt252,
/// Name of the contract.
pub name: ByteArray,
/// Calldata for dojo_init.
pub init_calldata: Span<felt252>,
/// The contract to grant permission to.
pub contract: ContractDescriptor,
/// Selectors of the resources that the contract is granted writer access to.
pub writer_of: Span<felt252>,
/// Selector of the resource that the contract is the owner of.
pub owner_of: Span<felt252>,
/// Calldata for dojo_init.
pub init_calldata: Span<felt252>,
}

#[derive(Drop)]
pub struct NamespaceDef {
pub namespace: ByteArray,
pub resources: Span<TestResource>,
}

#[generate_trait]
pub impl ContractDefImpl of ContractDefTrait {
fn new(class_hash: felt252, name: ByteArray) -> ContractDef {
fn new(namespace: @ByteArray, name: @ByteArray,) -> ContractDef {
ContractDef {
class_hash, name, init_calldata: [].span(), writer_of: [].span(), owner_of: [].span()
contract: ContractDescriptor::Named((namespace, name)),
writer_of: [].span(),
owner_of: [].span(),
init_calldata: [].span()
}
}

Expand Down Expand Up @@ -92,6 +112,10 @@ pub fn deploy_with_world_address(class_hash: felt252, world: IWorldDispatcher) -

/// Spawns a test world registering provided resources into namespaces.
///
/// This function only deploys the world and registers the resources, it does not initialize the
/// contracts or any permissions.
/// The first namespace is used as the default namespace when [`WorldStorage`] is returned.
///
/// # Arguments
///
/// * `namespaces_defs` - Definitions of namespaces to register.
Expand Down Expand Up @@ -122,37 +146,64 @@ pub fn spawn_test_world(namespaces_defs: Span<NamespaceDef>) -> WorldStorage {
first_namespace = Option::Some(namespace.clone());
}

let namespace_hash = dojo::utils::bytearray_hash(@namespace);

for r in ns
.resources
.clone() {
match r {
TestResource::Event(ch) => { world.register_event(namespace.clone(), *ch); },
TestResource::Model(ch) => { world.register_model(namespace.clone(), *ch); },
TestResource::Contract(def) => {
let class_hash: ClassHash = (*def.class_hash).try_into().unwrap();
let contract_address = world
.register_contract(*def.class_hash, namespace.clone(), class_hash);

for target in *def
.writer_of {
world.grant_writer(*target, contract_address);
};

for target in *def
.owner_of {
world.grant_owner(*target, contract_address);
};

let selector = dojo::utils::selector_from_namespace_and_name(
namespace_hash, def.name
);
world.init_contract(selector, *def.init_calldata);
TestResource::Event(ch) => {
world.register_event(namespace.clone(), (*ch).try_into().unwrap());
},
TestResource::Model(ch) => {
world.register_model(namespace.clone(), (*ch).try_into().unwrap());
},
TestResource::Contract(ch) => {
world.register_contract(*ch, namespace.clone(), (*ch).try_into().unwrap());
}
Comment on lines +174 to +176
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Parameter Mismatch in register_contract Call

Ohayo sensei! After reviewing the IWorld trait definition, I can confirm there is indeed an issue. The register_contract function expects three parameters:

  • salt: felt252
  • namespace: ByteArray
  • class_hash: ClassHash

However, in the code:

world.register_contract(*ch, namespace.clone(), (*ch).try_into().unwrap());

The same class hash value *ch is being used both as the salt and the class hash, which is incorrect. The salt should be a unique value to determine the contract's address.

Looking at other usages in the codebase, they typically use a literal salt value like 'salt1':

world.register_contract('salt1', namespace, class_hash);
🔗 Analysis chain

Potential Issue with world.register_contract Parameters

Ohayo sensei! There might be a concern with the parameters passed to world.register_contract in the spawn_test_world function. Specifically, the class hash *ch is being used both as the contract address and the class hash:

world.register_contract(*ch, namespace.clone(), (*ch).try_into().unwrap());

Since *ch is a TestClassHash (a felt252 representing the class hash), using it as the contract address may not be correct. Please verify that the correct contract address is being provided.

To ensure that register_contract receives the correct parameters, you can run the following script to check where register_contract is defined and how it's used elsewhere:

Let me check the interface definition to understand the expected parameter types.

Let me try a different approach to find the interface definition.

Let me search specifically for the register_contract function in the IWorld trait.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all usages of `world.register_contract` to verify correct parameter usage.

# Search for all occurrences of `register_contract` in the codebase
rg 'register_contract\(' -A 2

Length of output: 7841

Script:

#!/bin/bash
# Find the interface definition for register_contract
ast-grep --pattern 'fn register_contract($$$)'

Length of output: 48

Script:

#!/bin/bash
# Search for the interface definition in the core module
rg -A 5 'trait IWorld' crates/dojo/core/src/

Length of output: 1428

Script:

#!/bin/bash
# Search for register_contract function in IWorld trait
rg -B 2 -A 4 'fn register_contract' crates/dojo/core/src/world/iworld.cairo

Length of output: 406

}
}
};

WorldStorageTrait::new(world, @first_namespace.unwrap())
}

#[generate_trait]
pub impl WorldStorageInternalTestImpl of WorldStorageTestTrait {
fn sync_perms_and_inits(self: @WorldStorage, contracts: Span<ContractDef>) {
// First, sync permissions as sozo is doing.
for c in contracts {
let contract_address = match c.contract {
ContractDescriptor::Address(address) => *address,
ContractDescriptor::Named((
namespace, name
)) => {
let selector = dojo::utils::selector_from_names(*namespace, *name);
match (*self.dispatcher).resource(selector) {
dojo::world::Resource::Contract((address, _)) => address,
_ => panic!("Contract not found"),
}
},
};

for w in *c.writer_of {
(*self.dispatcher).grant_writer(*w, contract_address);
};

for o in *c.owner_of {
(*self.dispatcher).grant_owner(*o, contract_address);
};
};

// Then, calls the dojo_init for each contract that is a dojo contract.
for c in contracts {
match c.contract {
ContractDescriptor::Address(_) => {},
ContractDescriptor::Named((
namespace, name
)) => {
let selector = dojo::utils::selector_from_names(*namespace, *name);
(*self.dispatcher).init_contract(selector, *c.init_calldata);
}
}
};
}
}
18 changes: 11 additions & 7 deletions examples/simple/src/lib.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,24 +115,28 @@ pub mod c3 {}
#[cfg(test)]
mod tests {
use dojo::model::ModelStorage;
use dojo_cairo_test::{spawn_test_world, NamespaceDef, TestResource, ContractDefTrait};
use dojo_cairo_test::{
spawn_test_world, NamespaceDef, TestResource, ContractDefTrait, WorldStorageTestTrait
};
use super::{c1, m_M, M};

#[test]
fn test_1() {
let ndef = NamespaceDef {
namespace: "ns", resources: [
TestResource::Model(m_M::TEST_CLASS_HASH.try_into().unwrap()),
TestResource::Contract(
ContractDefTrait::new(c1::TEST_CLASS_HASH, "c1")
.with_init_calldata([0xff].span())
.with_writer_of([dojo::utils::bytearray_hash(@"ns")].span())
)
TestResource::Model(m_M::TEST_CLASS_HASH),
TestResource::Contract(c1::TEST_CLASS_HASH),
].span()
};

let world = spawn_test_world([ndef].span());

let c1_def = ContractDefTrait::new(@"ns", @"c1")
.with_writer_of([dojo::utils::bytearray_hash(@"ns")].span())
.with_init_calldata([0xff].span());

world.sync_perms_and_inits([c1_def].span());

let m: M = world.read_model(0);
assert!(m.v == 0xff, "invalid b");
//let m2 = M { a: 120, b: 244, };
Expand Down
26 changes: 18 additions & 8 deletions examples/spawn-and-move/src/actions.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,34 +210,43 @@ pub mod actions {
mod tests {
use dojo::model::{ModelStorage, ModelValueStorage, ModelStorageTest};
use dojo::world::WorldStorageTrait;
use dojo_cairo_test::{spawn_test_world, NamespaceDef, TestResource, ContractDefTrait};
use dojo_cairo_test::{
spawn_test_world, NamespaceDef, TestResource, ContractDefTrait, ContractDef,
WorldStorageTestTrait
};

use super::{actions, IActionsDispatcher, IActionsDispatcherTrait};
use dojo_examples::models::{Position, PositionValue, m_Position, Moves, m_Moves, Direction,};

fn namespace_def() -> NamespaceDef {
let ndef = NamespaceDef {
namespace: "ns", resources: [
TestResource::Model(m_Position::TEST_CLASS_HASH.try_into().unwrap()),
TestResource::Model(m_Moves::TEST_CLASS_HASH.try_into().unwrap()),
TestResource::Event(actions::e_Moved::TEST_CLASS_HASH.try_into().unwrap()),
TestResource::Contract(
ContractDefTrait::new(actions::TEST_CLASS_HASH, "actions")
.with_writer_of([dojo::utils::bytearray_hash(@"ns")].span())
)
TestResource::Model(m_Position::TEST_CLASS_HASH),
TestResource::Model(m_Moves::TEST_CLASS_HASH),
TestResource::Event(actions::e_Moved::TEST_CLASS_HASH),
TestResource::Contract(actions::TEST_CLASS_HASH),
].span()
};

ndef
}

fn contract_defs() -> Span<ContractDef> {
[
ContractDefTrait::new(@"ns", @"actions")
.with_writer_of([dojo::utils::bytearray_hash(@"ns")].span())
].span()
}

#[test]
fn test_world_test_set() {
let caller = starknet::contract_address_const::<0x0>();

let ndef = namespace_def();
let mut world = spawn_test_world([ndef].span());

world.sync_perms_and_inits(contract_defs());

// Without having the permission, we can set data into the dojo database for the given
// models.
let mut position: Position = world.read_model(caller);
Expand Down Expand Up @@ -272,6 +281,7 @@ mod tests {

let ndef = namespace_def();
let mut world = spawn_test_world([ndef].span());
world.sync_perms_and_inits(contract_defs());

let (actions_system_addr, _) = world.dns(@"actions").unwrap();
let actions_system = IActionsDispatcher { contract_address: actions_system_addr };
Expand Down
Loading