feat: use of `WorldContractReader` from `abigen!` #1010

glihm · 2023-10-11T03:23:33Z

This PR aims at exploring how abigen! macro in PR on starknet-rs can be used to reduce amount of code related to contract calls / invokes.

Few points here:

abigen! is still experimental, but thanks to this PR I was able to fix several important things. Thank you dojo! :)
Multi-call is not yet supported by abigen, so some functions were not modified. However, if we can add options to the macro, an option like external_generate_call=true can trigger an additional code generation for externals like my_external_call which returns the Call instead of actually executing the transaction. This will allow the combinations of several Calls in only one execute, leveraging abigen rust string typing. Example where it can be useful in torii: register_models.
I renamed the methods that had the same name as contract method with from_str as there are some additional parameter checking based on input string. Does this sound good to you too?
I do like how you were working with Contract and ContractReader, I've ported the same implementation in the abigen which in my opinion is a very good approach. This allows the current code to directly add some impl for the structs generated by abigen which have the same name as the one before.
With JSON files, I have some problem with the paths, were clippy is expecting a relative path to the Cargo.toml of torii-client, but cargo build -p torii-client is expecting a relative path to Cargo.toml of the workspace. Keep investigating, but any help on this would be very welcomed!
To generate the abi, I have to add this to the Scarb.toml:

[[target.starknet-contract]]
sierra = true

Is it ok to push this change, or we do not want this into the scarb manifest?

I'll keep this PR in draft for now to gather some feedback on this work. Any comment would be greatly appreciated to keep exploring this path and enhancing robustness and capabilities of abigen.

tarrencev

Looks good! Can we remove the from_str methods if we pass a FieldElement instead?

tarrencev · 2023-10-17T01:28:38Z

crates/torii/core/src/processors/register_model.rs

-        let model = world.model(&name, BlockId::Tag(BlockTag::Latest)).await?;
-        let schema = model.schema(BlockId::Tag(BlockTag::Latest)).await?;
-        let layout = model.layout(BlockId::Tag(BlockTag::Latest)).await?;
+        world.set_call_block_id(BlockId::Tag(BlockTag::Latest));


Hm this is an interesting pattern. Why did you decide to go this path vs passing for each call?

Adding an extra-parameter to function in a sense makes it less natural to call a function of our contract as we have to know that the abigen will add this extra param.

But at the same time, having the set_call_block_id pattern is a "limitation"/"constraint" when the instance is shared, as we need a mut reference to adjust the block id, and this may cause some races without adding mutex for instance.

So... I would say that it was an experiment, and after making more test I am not sure what can be the best approach. Or maybe abigen may give this option to the user?

glihm · 2023-10-17T17:57:29Z

Looks good! Can we remove the from_str methods if we pass a FieldElement instead?

If there is no problem at externalizing the inputs verification, we can definitely remove the from_str methods to keep using only the generated code.

Taking this example:

pub async fn grant_writer_from_str(
    &self,
    model: &str,
    contract: &ContractAddress,
) -> Result<
    InvokeTransactionResult,
    WorldContractError<A::SignError, <A::Provider as Provider>::Error>,
> {
    let model = cairo_short_string_to_felt(model)
        .map_err(WorldContractError::CairoShortStringToFeltError)?;

    self.grant_writer(&model, contract).await.map_err(WorldContractError::AccountError)
}

The model verification should then be done out of this function by the caller of the generated grant_writer function.

tarrencev · 2023-10-29T00:57:08Z

Looks good! Can we remove the from_str methods if we pass a FieldElement instead?

If there is no problem at externalizing the inputs verification, we can definitely remove the from_str methods to keep using only the generated code.

Taking this example:
pub async fn grant_writer_from_str(
    &self,
    model: &str,
    contract: &ContractAddress,
) -> Result<
    InvokeTransactionResult,
    WorldContractError<A::SignError, <A::Provider as Provider>::Error>,
> {
    let model = cairo_short_string_to_felt(model)
        .map_err(WorldContractError::CairoShortStringToFeltError)?;

    self.grant_writer(&model, contract).await.map_err(WorldContractError::AccountError)
}
The model verification should then be done out of this function by the caller of the generated grant_writer function.

I think this is fine, until cairo gets a string type which can handle validation internally

glihm · 2023-12-16T05:27:12Z

@tarrencev if you have time I would appreciate a new review on this one. 🙏

(Contains the commit from #1297 as I used it for debug, I can rebase depending on which is merged first)

glihm · 2023-12-16T05:33:40Z

I'm thinking of generating the two .json file for world and executor during the CI. This can help to prevent someone changing the cairo files, without actually updating the artifacts.

This will also help us ensuring that the rust programs correctly use the cairo methods.

What is the best for that, adding a step in the CI and use sozo to build the spawn-and-move example? I didn't see any build step, does the devcontainer already contain compiled artifacts?

tarrencev · 2023-12-18T19:52:34Z

I'm thinking of generating the two .json file for world and executor during the CI. This can help to prevent someone changing the cairo files, without actually updating the artifacts.

This will also help us ensuring that the rust programs correctly use the cairo methods.

What is the best for that, adding a step in the CI and use sozo to build the spawn-and-move example? I didn't see any build step, does the devcontainer already contain compiled artifacts?

This is currently done here: https://github.com/dojoengine/dojo/blob/main/crates/dojo-test-utils/build.rs

codecov-commenter · 2023-12-20T04:04:51Z

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (bc42b8e) 59.24% compared to head (3bfcda9) 60.43%.
Report is 6 commits behind head on main.

❗ Current head 3bfcda9 differs from pull request most recent head 62446b7. Consider uploading reports for the commit 62446b7 to get more accurate results

Files	Patch %	Lines
crates/sozo/src/ops/register.rs	0.00%	7 Missing ⚠️
crates/dojo-world/src/contracts/cairo_utils.rs	73.68%	5 Missing ⚠️
crates/dojo-world/src/contracts/model.rs	78.57%	3 Missing ⚠️
crates/sozo/src/ops/model.rs	0.00%	3 Missing ⚠️
crates/torii/client/src/client/mod.rs	0.00%	3 Missing ⚠️
crates/sozo/src/ops/auth.rs	0.00%	2 Missing ⚠️
crates/sozo/src/ops/migration/mod.rs	85.71%	2 Missing ⚠️
crates/dojo-world/src/contracts/world.rs	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1010      +/-   ##
==========================================
+ Coverage   59.24%   60.43%   +1.18%     
==========================================
  Files         225      226       +1     
  Lines       18900    18777     -123     
==========================================
+ Hits        11198    11347     +149     
+ Misses       7702     7430     -272

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

glihm · 2023-12-20T04:07:13Z

Due to the dependencies between dojo-world and dojo-lang which are used into dojo-test-utils to actually build the artifacts, the PR for now embed the ABI in the .git.

Inside the dojo-world/src/contracts/abi there is a README to ensure any dev can generate them in case the ABI changes. This anyway will be catch up by the compilation if the bindings are not correct.

@kariy @tarrencev let me know if you have a chance to review and if some stuff here must be re-considered. 🙏

tarrencev

really nice improvement

crates/dojo-lang/src/manifest_test_data/compiler_cairo_v240/Scarb.lock

crates/torii/types-test/Scarb.lock

crates/dojo-world/src/contracts/world.rs

crates/dojo-world/src/contracts/cairo_utils.rs

glihm changed the title ~~feat: use of world from abigen!~~ feat: use of WorldContractReader from abigen! Oct 11, 2023

tarrencev reviewed Oct 17, 2023

View reviewed changes

tarrencev force-pushed the main branch 2 times, most recently from 0f4d258 to 19d76ce Compare October 20, 2023 21:09

tarrencev force-pushed the main branch 9 times, most recently from dd36cf5 to 4979471 Compare December 1, 2023 08:56

tarrencev force-pushed the main branch 8 times, most recently from 6a784ce to a08f2cb Compare December 10, 2023 03:06

glihm added 2 commits December 15, 2023 17:17

feat: add cainome abigen to sozo

f67451e

feat: add cainome abigen to torii

71a1830

glihm force-pushed the feat/world-abigen branch from 45c5558 to 71a1830 Compare December 15, 2023 23:34

glihm added 4 commits December 15, 2023 17:49

fix: adjust test and fmt

da371f0

fix: remove additional calldata input

80294d3

feat(katana-rpc): add ContractErrorData when it applies

17f895c

fix: fix tests

f08a2d7

fix: adjust indexes for executor call results

e9124df

glihm marked this pull request as ready for review December 16, 2023 05:25

glihm requested a review from tarrencev December 16, 2023 05:33

glihm added 2 commits December 17, 2023 15:39

fix: restore Scarb.lock and add .tool-versions

f24f10f

chore: bump cainome to 0.1.5

6adc6f6

tarrencev force-pushed the main branch from a4fc4f9 to 9a0421f Compare December 19, 2023 01:18

glihm added 4 commits December 19, 2023 21:35

fix: embed world and executor ABI in .git

70347fc

chore: bump cainome to 0.1.7

499ff25

chore: update Scarb.lock

3211d1c

docs: update README for contract ABI

6e7ceac

glihm force-pushed the feat/world-abigen branch from cdc91b8 to 6e7ceac Compare December 20, 2023 03:38

glihm added 2 commits December 19, 2023 21:41

merge main

805740a

fix: fmt clippy from merge

c34e516

tarrencev approved these changes Dec 20, 2023

View reviewed changes

crates/dojo-lang/src/manifest_test_data/compiler_cairo_v240/Scarb.lock Outdated Show resolved Hide resolved

crates/torii/types-test/Scarb.lock Outdated Show resolved Hide resolved

kariy requested changes Dec 20, 2023

View reviewed changes

crates/dojo-world/src/contracts/world.rs Outdated Show resolved Hide resolved

crates/dojo-world/src/contracts/cairo_utils.rs Outdated Show resolved Hide resolved

glihm added 3 commits December 20, 2023 09:53

fix: fix PR comments

74aa54c

fix: fix typo after test being run

3bfcda9

fix: fix Scarb.lock version

62446b7

glihm requested a review from kariy December 20, 2023 15:59

kariy approved these changes Dec 20, 2023

View reviewed changes

glihm merged commit 792194c into dojoengine:main Dec 20, 2023
9 checks passed

glihm deleted the feat/world-abigen branch December 20, 2023 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use of `WorldContractReader` from `abigen!` #1010

feat: use of `WorldContractReader` from `abigen!` #1010

glihm commented Oct 11, 2023 •

edited

Loading

tarrencev left a comment

tarrencev Oct 17, 2023

glihm Oct 17, 2023

glihm commented Oct 17, 2023 •

edited

Loading

tarrencev commented Oct 29, 2023

glihm commented Dec 16, 2023 •

edited

Loading

glihm commented Dec 16, 2023

tarrencev commented Dec 18, 2023

codecov-commenter commented Dec 20, 2023 •

edited

Loading

glihm commented Dec 20, 2023

tarrencev left a comment

feat: use of WorldContractReader from abigen! #1010

feat: use of WorldContractReader from abigen! #1010

Conversation

glihm commented Oct 11, 2023 • edited Loading

tarrencev left a comment

Choose a reason for hiding this comment

tarrencev Oct 17, 2023

Choose a reason for hiding this comment

glihm Oct 17, 2023

Choose a reason for hiding this comment

glihm commented Oct 17, 2023 • edited Loading

tarrencev commented Oct 29, 2023

glihm commented Dec 16, 2023 • edited Loading

glihm commented Dec 16, 2023

tarrencev commented Dec 18, 2023

codecov-commenter commented Dec 20, 2023 • edited Loading

Codecov Report

glihm commented Dec 20, 2023

tarrencev left a comment

Choose a reason for hiding this comment

feat: use of `WorldContractReader` from `abigen!` #1010

feat: use of `WorldContractReader` from `abigen!` #1010

glihm commented Oct 11, 2023 •

edited

Loading

glihm commented Oct 17, 2023 •

edited

Loading

glihm commented Dec 16, 2023 •

edited

Loading

codecov-commenter commented Dec 20, 2023 •

edited

Loading