feat: add [dojo::interface] attribute #1594

remybar · 2024-03-01T14:46:35Z

Related to DOJ-132, #1458.

Add a new [dojo::interface] attribute which is expanded in [starknet::interface], allowing the user to ignore TContractState type and self parameter.

remybar · 2024-03-01T14:49:13Z

Here the first part of the issue related to [dojo::interface].

As noted in the code, I ignored some function parts (like nopanic clause) as, afaik, there are not possible in a starknet::interface trait.
I was wondering if I can improve the indentation of the generated code or if it's not important 🤔

codecov · 2024-03-02T03:37:41Z

Codecov Report

Attention: Patch coverage is 96.54179% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 69.55%. Comparing base (cf095de) to head (5415aef).
Report is 2 commits behind head on main.

Files	Patch %	Lines
crates/dojo-lang/src/contract.rs	94.11%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1594      +/-   ##
==========================================
+ Coverage   69.09%   69.55%   +0.45%     
==========================================
  Files         265      266       +1     
  Lines       26859    27213     +354     
==========================================
+ Hits        18559    18927     +368     
+ Misses       8300     8286      -14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

glihm

First quick review of the draft, very nice start!
Left only minor comments for now, good job!

crates/dojo-lang/src/interface.rs

remybar · 2024-03-02T09:19:40Z

First quick review of the draft, very nice start! Left only minor comments for now, good job!

Thanks ser ! 🙏

I don't know why but if a trait contains other elements than a function, when I copy these elements with vec![RewriteNode::Copied(el.as_syntax_node())], they are wrongly copied 🤔

For example, I tried with a constant const ONE: u8 = 1; and it became const ONE ...

dojo/crates/dojo-lang/src/interface.rs

Lines 28 to 44 in a3dca4a

    
                   if let MaybeTraitBody::Some(body) = trait_ast.body(db) { 
        
                       let body_nodes: Vec<_> = body 
        
                           .items(db) 
        
                           .elements(db) 
        
                           .iter() 
        
                           .flat_map(|el| { 
        
                                 if let ast::TraitItem::Function(fn_ast) = el { 
        
                                   return system.rewrite_function(db, fn_ast.clone()); 
        
                               } 
        
                               //TODO: if the trait body contains other things than functions,  
        
                               // they are not correctly copied. 
        
                               // for example: `const ONE: u8 = 1;` is copied as `const ONE` 
        
           //                    vec![RewriteNode::Copied(el.as_syntax_node())] 
        
                               vec![] 
        
                           }) 
        
                           .collect();

crates/dojo-lang/src/interface.rs

remybar · 2024-03-03T13:57:16Z

I rewrote the code for dojo:interface to make it easier to read and to not have to handle corner cases. The idea is to patch the smallest parts of code.

I updated the contract.rs file to handle self and world parameters in free functions and functions inside Impl block.

I still need to look at a weird issue when I add a constant in a trait with the [starknet::interface]] attribute. I got a = 1 at the beginning of the expanded trait code 🤔
In fact, I'm not sure we can add a constant in a trait (I was not able to compile a Cairo project with this kind of thing), but the parser accepts other things than functions in a trait ... And to have a 100% code coverage for the CI, I need to test that branch with the diagnostic logging 😅

Now, I will also have a look at CI errors ;-)

remybar · 2024-03-04T04:12:25Z

I still need to look at a weird issue when I add a constant in a trait with the [starknet::interface]] attribute. I got a = 1 at the beginning of the expanded trait code 🤔 In fact, I'm not sure we can add a constant in a trait (I was not able to compile a Cairo project with this kind of thing), but the parser accepts other things than functions in a trait ... And to have a 100% code coverage for the CI, I need to test that branch with the diagnostic logging 😅

After some tests, I found that if I add const ONE: u8 = 1; at the beginning of a trait, =1; is parsed as a trait function keyword.
So, I searched directly in the cairo parser, and it appears that the expected pattern is const <name>: type; (see here).
I don't know what a const without a value means but I propose to let const ONE: u8; in the system file to just test the diagnostic message.

glihm

@remybar thanks for the update!
A small comment about testing.

Also, would you mind updating the spawn-and-move example with the new syntax? This example is usually taken as an example and showcasing the dojo stack. :)

crates/dojo-lang/src/plugin_test_data/system

Add a new `[dojo::interface]` attribute which is expanded in `[starknet::interface]`, allowing the user to ignore `TContractState` type and `self` parameter.

`self` parameter is not mandatory anymore in dojo::contract. If not present, it will be automatically added during code expansion. if a `world: IWorldDispatcher` parameter is given, it is removed and replace by the `let world = self.world_dispatcher.read();` statement at the beginning of the function body.

remybar · 2024-03-06T02:23:59Z

Also, would you mind updating the spawn-and-move example with the new syntax? This example is usually taken as an example and showcasing the dojo stack. :)

Sure ! I updated the example 👍

Thanks for the review !

glihm · 2024-03-06T03:28:53Z

crates/dojo-lang/src/plugin_test_data/system

+trait INominalTrait {
+    fn do_no_param();
+    fn do_no_param_but_self(self: @TContractState);
+    fn do_params(p1: dojo_examples::models::Direction, p2: u8);
+    fn do_params_and_self(self: @TContractState, p2: u8);
+    fn do_return_value(p1: u8) -> u16;
+    fn do_ref_self(ref self: TContractState);
+
+    #[my_attr]
+    fn do_with_attrs(p1: u8) -> u16;
+}


We still miss a non-faulty test case, no? The ref self and the #[my_attr] should go into the faulty one, or you're thinking of adding one more?

Ah yes, I will move ref self to the faulty trait !

For #[my_attr], from the code expansion point of view, this is not an error. If a function has some attributes, they will be copied. But from Dojo point of view, maybe it's strange to have some attributes on functions in a contract trait ? So, if you think it is better to move this case in the faulty trait, I will do it 👍

glihm · 2024-03-06T03:33:20Z

examples/spawn-and-move/src/actions.cairo

@@ -83,8 +82,7 @@ mod actions {
            );
        }

-        fn move(self: @ContractState, direction: Direction) {
-            let world = self.world_dispatcher.read();
+        fn move(direction: Direction, world: IWorldDispatcher) {


Seeing this, we must inline and remove the world: IWorldDispatcher from arguments if and only if it's the first argument. We shouldn't allow two arguments with this type either (an other faulty one to add! hehe).

Because at ABI level, removing world at first position can be understood. If it's randomly placed in the list of inputs of the function, it can quickly become difficult to call the systems.

Thanks ! I will update the code accordingly 👍

examples/spawn-and-move/src/actions.cairo

Co-authored-by: glihm <dev@glihm.net>

remybar · 2024-03-06T11:51:26Z

So, to summarize:

[dojo::interface]
- add <TContractState> to the trait name,
- add self parameter if not already present as first parameter,
- raise a diagnostic error if there is a ref self parameter in at least one function,
- raise a diagnostic error if there is other things than functions in the trait.
[dojo::contract]
- add self parameter if not already present as first parameter,
- remove the world: IWorldDispatcher parameter if it is the first parameter of the function (self excluded) and if there is no more than one parameter of type IWorldDispatcher,
- inline the world parameter by adding the let world = self.world_dispatcher.read(); as first function statement if this world parameter has been removed,
- raise a diagnostic error if there is a ref self parameter in at least one function,
- raise a diagnostic error if there is more than one parameter of type IWorldDispatcher for a function.

glihm

Awesome work man, some comments here that I think should be almost the latest on my end. It looks great!

@tarrencev when commenting on the issue I didn't get well the fact that the world is only injected in the impl. The interface is super clean and the Devex is very nice.

crates/dojo-lang/src/contract.rs

glihm · 2024-03-08T06:11:30Z

crates/dojo-lang/src/plugin_test_data/system

+        'land'
+    }
+
+    #[external(v0)]


abi(embed_v0) is a bit more painful to use as it can't be used on standalone function. But external(v0) is expected to be deprecated. Would it be possible to switch to abi(embed_v0) in this PR?

Hum, from what I understand, these attributes are not really important in this system file because they are not really used by the plugin to generate some specific code. But it is better to have at least one attribute on each functions to be sure that function attributes are well copied in the output code.

I've updated the code to use abi(embed_v0) on Impl block and external(v0) on free functions. What should we use instead of external(v0) for free functions ? Or do we have to put external/public functions in a Impl block to have them exported in the ABI file maybe ?

crates/dojo-lang/src/plugin_test_data/system

remybar force-pushed the feat-improve_syntax branch from b739cad to 4d10c48 Compare March 2, 2024 02:25

glihm mentioned this pull request Mar 2, 2024

refactor: migrate spawn-and-move to new proposed syntax #1457

Closed

glihm reviewed Mar 2, 2024

View reviewed changes

crates/dojo-lang/src/interface.rs Outdated Show resolved Hide resolved

crates/dojo-lang/src/interface.rs Outdated Show resolved Hide resolved

remybar force-pushed the feat-improve_syntax branch from 4d10c48 to a3dca4a Compare March 2, 2024 09:00

tarrencev reviewed Mar 2, 2024

View reviewed changes

crates/dojo-lang/src/interface.rs Outdated Show resolved Hide resolved

crates/dojo-lang/src/interface.rs Outdated Show resolved Hide resolved

crates/dojo-lang/src/interface.rs Outdated Show resolved Hide resolved

remybar force-pushed the feat-improve_syntax branch 2 times, most recently from ef91c9b to c7fc610 Compare March 3, 2024 13:52

remybar force-pushed the feat-improve_syntax branch from c7fc610 to 6c28119 Compare March 3, 2024 14:50

remybar marked this pull request as ready for review March 4, 2024 16:38

remybar force-pushed the feat-improve_syntax branch from eb089be to 14bd96f Compare March 5, 2024 00:05

glihm reviewed Mar 5, 2024

View reviewed changes

crates/dojo-lang/src/plugin_test_data/system Outdated Show resolved Hide resolved

Rémy Baranx and others added 6 commits March 6, 2024 08:13

feat: add [dojo::interface] attribute

52a5a50

Add a new `[dojo::interface]` attribute which is expanded in `[starknet::interface]`, allowing the user to ignore `TContractState` type and `self` parameter.

add some context to explain the constant without value in system file

648e500

fix format

b63eb63

split test cases to have at least one fully working test case

ad5cc68

update spawn-and-move example

ce78cf1

remybar force-pushed the feat-improve_syntax branch from 14bd96f to ce78cf1 Compare March 6, 2024 02:22

glihm reviewed Mar 6, 2024

View reviewed changes

Update examples/spawn-and-move/src/actions.cairo

7449812

Co-authored-by: glihm <dev@glihm.net>

remybar force-pushed the feat-improve_syntax branch from 5273928 to a5cd4d5 Compare March 8, 2024 05:14

glihm reviewed Mar 8, 2024

View reviewed changes

update test cases and improve world parameter conditions

5b2eeef

remybar force-pushed the feat-improve_syntax branch from a5cd4d5 to 5b2eeef Compare March 8, 2024 09:57

fmt

5415aef

tarrencev merged commit ad60c39 into dojoengine:main Mar 8, 2024
12 checks passed

glihm mentioned this pull request Mar 12, 2024

Improve dojo contract syntax #1458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add [dojo::interface] attribute #1594

feat: add [dojo::interface] attribute #1594

remybar commented Mar 1, 2024 •

edited by glihm

Loading

remybar commented Mar 1, 2024

codecov bot commented Mar 2, 2024 •

edited

Loading

glihm left a comment

remybar commented Mar 2, 2024

remybar commented Mar 3, 2024 •

edited

Loading

remybar commented Mar 4, 2024 •

edited

Loading

glihm left a comment

remybar commented Mar 6, 2024

glihm Mar 6, 2024

remybar Mar 6, 2024

glihm Mar 6, 2024

remybar Mar 6, 2024

remybar commented Mar 6, 2024 •

edited

Loading

glihm left a comment

glihm Mar 8, 2024

remybar Mar 8, 2024

feat: add [dojo::interface] attribute #1594

feat: add [dojo::interface] attribute #1594

Conversation

remybar commented Mar 1, 2024 • edited by glihm Loading

remybar commented Mar 1, 2024

codecov bot commented Mar 2, 2024 • edited Loading

Codecov Report

glihm left a comment

Choose a reason for hiding this comment

remybar commented Mar 2, 2024

remybar commented Mar 3, 2024 • edited Loading

remybar commented Mar 4, 2024 • edited Loading

glihm left a comment

Choose a reason for hiding this comment

remybar commented Mar 6, 2024

glihm Mar 6, 2024

Choose a reason for hiding this comment

remybar Mar 6, 2024

Choose a reason for hiding this comment

glihm Mar 6, 2024

Choose a reason for hiding this comment

remybar Mar 6, 2024

Choose a reason for hiding this comment

remybar commented Mar 6, 2024 • edited Loading

glihm left a comment

Choose a reason for hiding this comment

glihm Mar 8, 2024

Choose a reason for hiding this comment

remybar Mar 8, 2024

Choose a reason for hiding this comment

remybar commented Mar 1, 2024 •

edited by glihm

Loading

codecov bot commented Mar 2, 2024 •

edited

Loading

remybar commented Mar 3, 2024 •

edited

Loading

remybar commented Mar 4, 2024 •

edited

Loading

remybar commented Mar 6, 2024 •

edited

Loading