feat: add [dojo::interface] attribute

[remybar]

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]

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]

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!

[remybar]

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();

[remybar]

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]

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. :)

[remybar]

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]

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?

[remybar]

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]

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.

[remybar]

Thanks ! I will update the code accordingly 👍

[remybar]

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.

[glihm]

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?

[remybar]

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 ?