feat: initial execution context implementation #392

ftupas · 2023-05-31T08:08:20Z

This pr does the following:

introduce the Context struct to system execution to provide the dojo execution context
update executor so it passes the context instead of the world_address
generation logic in system.rs and commands are updated to inject the context
use the execution context in auth checking
store execution role in world
resolve Introduce execution context #345

Note:

~~this pr doesn't implement authorization checking yet using the context~~
~~caller system is still stored in the world contract, will have to be refactored once context is used for auth~~

crates/dojo-core/src/execution_context.cairo

crates/dojo-core/src/interfaces.cairo

crates/dojo-core/src/executor.cairo

ftupas · 2023-05-31T08:13:10Z

crates/dojo-lang/src/system.rs

@@ -122,8 +123,7 @@ impl System {
        rewrite_nodes.push(RewriteNode::interpolate_patched(
            "
                #[external]
-                fn execute($parameters$$separator$world_address: starknet::ContractAddress) \
-             $ret_clause$ {
+                fn execute($parameters$$separator$ctx: Context) $ret_clause$ {


Use the context instead of world_address

ftupas · 2023-05-31T08:14:19Z

crates/dojo-lang/src/commands/execute.rs

-                 }.execute(dojo_core::string::ShortStringTrait::new('$system$'), $calldata$);
+                "let $var_name$ = \
+                 ctx.world.execute(dojo_core::string::ShortStringTrait::new('$system$'), \
+                 $role_id$, $calldata$);


Add role_id to execute command

tarrencev

looks really good so far!

tarrencev · 2023-05-31T17:52:52Z

crates/dojo-core/src/execution_context.cairo

+struct Context {
+    world: IWorldDispatcher, // Dispatcher to the world contract
+    caller_account: ContractAddress, // Address of the origin
+    caller_system: ClassHash, // Class hash of the calling system


I think we probably want a system name here as well, since the class can be changed

Suggested change

caller_system: ClassHash, // Class hash of the calling system

caller_system: ShortString, // Name of the calling system

caller_system_class_hash: ClassHash, // Class hash of the calling system

Ah yeah good catch, will change this!

crates/dojo-core/src/executor.cairo

ftupas

Thanks for the feedback and review @tarrencev !

crates/dojo-core/src/interfaces.cairo

ftupas · 2023-06-01T14:44:43Z

crates/dojo-core/src/execution_context.cairo

+struct Context {
+    world: IWorldDispatcher, // Dispatcher to the world contract
+    caller_account: ContractAddress, // Address of the origin
+    caller_system: ClassHash, // Class hash of the calling system


Ah yeah good catch, will change this!

crates/dojo-core/src/executor.cairo

crates/dojo-core/src/auth/systems.cairo

crates/dojo-core/src/interfaces.cairo

crates/dojo-core/src/world.cairo

tarrencev · 2023-06-02T00:59:34Z

crates/dojo-core/src/world.cairo

+        if role_id == 'Admin'.into() {
+            assert(is_account_admin(), 'only admin can set Admin role');
+        }
+        execution_role::write(system, role_id);


hmm do we need the system here?

i think every system execute call could be proceeded by a assume_role call and i think we want to validate that the caller has permission to assume the role here?

that way, if there are multiple executions in a multicall, the caller can, for example, assume a role once that can be used across all the execute calls

You're right good catch, I'm removing the system here and we just check if the caller has this permission to assume the role 👍 also this way, the role can be used across multiple system calls

ftupas

Thanks for the feedback and review @tarrencev !

crates/dojo-core/src/auth/systems.cairo

ftupas · 2023-06-02T22:58:56Z

crates/dojo-core/src/world.cairo

+        if role_id == 'Admin'.into() {
+            assert(is_account_admin(), 'only admin can set Admin role');
+        }
+        execution_role::write(system, role_id);


You're right good catch, I'm removing the system here and we just check if the caller has this permission to assume the role 👍 also this way, the role can be used across multiple system calls

tarrencev

another banger

great job

- Remove passing of role from world interface - Move calling system from storage to context - Implement interface for setting of execution role - Update authorization check - use default role if not set

tarrencev · 2023-06-05T21:11:25Z

crates/dojo-core/src/world.cairo

+        if role_id == 'Admin'.into() {
+            assert(is_account_admin(), 'only admin can set Admin role');
+        }
+        let caller = get_tx_info().unbox().account_contract_address;


I think we need to validate the user can assume this role here still right?

ftupas commented May 31, 2023

View reviewed changes

tarrencev reviewed May 31, 2023

View reviewed changes

ftupas commented Jun 1, 2023

View reviewed changes

ftupas force-pushed the feat/initial-execution-context-implementation branch from 0d5395c to e072fbc Compare June 1, 2023 23:38

ftupas requested a review from tarrencev June 1, 2023 23:41

ftupas force-pushed the feat/initial-execution-context-implementation branch from e072fbc to ab82d12 Compare June 2, 2023 00:34

tarrencev reviewed Jun 2, 2023

View reviewed changes

ftupas force-pushed the feat/initial-execution-context-implementation branch from ab82d12 to 510fa69 Compare June 2, 2023 22:44

ftupas commented Jun 2, 2023

View reviewed changes

tarrencev approved these changes Jun 3, 2023

View reviewed changes

ftupas added 14 commits June 5, 2023 17:04

feat: add execution context

7a5281b

feat: impl Serde for IWorldDispatcher

09d7be2

feat: add role id for execution

a00e92b

feat: pass execution context naively

1895c2e

chore: minor edits

611d0f1

update: fix execute arguments

b7e3694

feat: use system name over class hash

244d1c4

feat: allow setting of execution role

84dba8a

- Remove passing of role from world interface - Move calling system from storage to context - Implement interface for setting of execution role - Update authorization check - use default role if not set

chore: fix getter name

07447b9

dev: move context as first parameter

1d57c1b

feat: make context passing more explicit

b82eed5

todo: remove serde impl for dispatchers once derivable

fa43ace

dev: use caller for assuming execution role

72170d7

chore: remove unused separator

1908bec

tarrencev force-pushed the feat/initial-execution-context-implementation branch from 835ac01 to 1908bec Compare June 5, 2023 21:10

tarrencev reviewed Jun 5, 2023

View reviewed changes

Fix rebase issue

edd8ce9

tarrencev merged commit 9c3d6ad into dojoengine:main Jun 6, 2023

ftupas mentioned this pull request Jun 7, 2023

[Dojo] Refactor authorization system into the world directly #435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial execution context implementation #392

feat: initial execution context implementation #392

ftupas commented May 31, 2023 •

edited

Loading

ftupas May 31, 2023

ftupas May 31, 2023

tarrencev left a comment

tarrencev May 31, 2023

ftupas Jun 1, 2023

ftupas left a comment

ftupas Jun 1, 2023

tarrencev Jun 2, 2023

ftupas Jun 2, 2023

ftupas left a comment

ftupas Jun 2, 2023

tarrencev left a comment

tarrencev Jun 5, 2023

	caller_system: ClassHash, // Class hash of the calling system
	caller_system: ShortString, // Name of the calling system
	caller_system_class_hash: ClassHash, // Class hash of the calling system

feat: initial execution context implementation #392

feat: initial execution context implementation #392

Conversation

ftupas commented May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrencev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftupas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftupas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrencev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftupas commented May 31, 2023 •

edited

Loading