AbstractSDK / cw-orchestrator

All-in-one Rust-based CosmWasm testing, scripting, and deployment tool.
https://orchestrator.abstract.money
GNU General Public License v3.0
75 stars 17 forks source link

Simplify the derived function arguments #399

Closed Kayanski closed 2 months ago

Kayanski commented 3 months ago

This Simplify the cw-orch contract function derives.

The functions do not take their argument types directly but an argument of the form Into<type> This allows for simpler interactions

Depends on https://github.com/AbstractSDK/cw-orchestrator/pull/398

Downside

The types have to be known when specifying the argument. This is specifically bad when using collect. They should be changed to collect::Vec<_>() for instance

Breaking changes

Here are the cases that WILL break when releasing this PR. Cw-orch-core is not breaking

cw-orch-fns-derive breaks

 // The received type is impl Into<Uint128>, so the type is impossible to derive
contract.mint(1278u128.into());
// Replaced by 
contract.mint(1278u128);
 // The received type is impl Into<Uint128>, so the type is impossible to derive
let mut receivers = HashSet::new();
receivers.insert("kayanski");
receivers.insert("misha");
// Just a conversion from HashSet to Vec
contract.multi_send(receivers.into_iter().collect());
// Replaced by 
contract.mint(receivers.into_iter().collect::<Vec<_>>());

Depends on https://github.com/AbstractSDK/cw-orchestrator/pull/398

cloudflare-pages[bot] commented 3 months ago

Deploying cw-orchestrator with  Cloudflare Pages  Cloudflare Pages

Latest commit: d43b34e
Status:⚡️  Build in progress...

View logs

Kayanski commented 2 months ago

I don't think we should add impl Into for every type of argument.

I suggest we add a match statement that only converts the arguments to impl Into<> for the types:

* Uint (all sizes)

* String

But not for

* Addr

* Custom structures

* booleans

This helps us keep the generated fns readable (as you can only see the generated code in your IDE).

Yes good idea ! We just need to keep in mind that because of how proc-macros work, we will not be able to accept such bounds on type aliases like :

pub type ChainId = String;
codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 98.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 56.0%. Comparing base (0a2f299) to head (d1b2e96). Report is 1 commits behind head on main.

:exclamation: Current head d1b2e96 differs from pull request most recent head d43b34e

Please upload reports for the commit d43b34e to get more accurate results.

Additional details and impacted files | [Files](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/399?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK) | Coverage Δ | | |---|---|---| | [contracts/mock\_contract/src/lib.rs](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/399?src=pr&el=tree&filepath=contracts%2Fmock_contract%2Fsrc%2Flib.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK#diff-Y29udHJhY3RzL21vY2tfY29udHJhY3Qvc3JjL2xpYi5ycw==) | `97.9% <100.0%> (ø)` | | | [contracts/mock\_contract/src/msg\_tests.rs](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/399?src=pr&el=tree&filepath=contracts%2Fmock_contract%2Fsrc%2Fmsg_tests.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK#diff-Y29udHJhY3RzL21vY2tfY29udHJhY3Qvc3JjL21zZ190ZXN0cy5ycw==) | `94.7% <100.0%> (ø)` | | | [contracts/mock\_contract\_u64/src/lib.rs](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/399?src=pr&el=tree&filepath=contracts%2Fmock_contract_u64%2Fsrc%2Flib.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK#diff-Y29udHJhY3RzL21vY2tfY29udHJhY3RfdTY0L3NyYy9saWIucnM=) | `82.7% <100.0%> (-0.4%)` | :arrow_down: | | [...ckages/macros/cw-orch-fns-derive/src/fns\_derive.rs](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/399?src=pr&el=tree&filepath=packages%2Fmacros%2Fcw-orch-fns-derive%2Fsrc%2Ffns_derive.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK#diff-cGFja2FnZXMvbWFjcm9zL2N3LW9yY2gtZm5zLWRlcml2ZS9zcmMvZm5zX2Rlcml2ZS5ycw==) | `100.0% <100.0%> (ø)` | | | [packages/macros/cw-orch-fns-derive/src/lib.rs](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/399?src=pr&el=tree&filepath=packages%2Fmacros%2Fcw-orch-fns-derive%2Fsrc%2Flib.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK#diff-cGFja2FnZXMvbWFjcm9zL2N3LW9yY2gtZm5zLWRlcml2ZS9zcmMvbGliLnJz) | `100.0% <ø> (ø)` | | | [packages/macros/cw-orch-fns-derive/src/helpers.rs](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/399?src=pr&el=tree&filepath=packages%2Fmacros%2Fcw-orch-fns-derive%2Fsrc%2Fhelpers.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK#diff-cGFja2FnZXMvbWFjcm9zL2N3LW9yY2gtZm5zLWRlcml2ZS9zcmMvaGVscGVycy5ycw==) | `78.7% <96.1%> (+5.5%)` | :arrow_up: | ... and [17 files with indirect coverage changes](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/399/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK)
Buckram123 commented 2 months ago

I don't think we should add impl Into for every type of argument. I suggest we add a match statement that only converts the arguments to impl Into<> for the types:

* Uint (all sizes)

* String

But not for

* Addr

* Custom structures

* booleans

This helps us keep the generated fns readable (as you can only see the generated code in your IDE).

Yes good idea ! We just need to keep in mind that because of how proc-macros work, we will not be able to accept such bounds on type aliases like :

pub type ChainId = String;

Is it possible to check it with typeid? https://doc.rust-lang.org/std/any/struct.TypeId.html

Kayanski commented 2 months ago

Is it possible to check it with typeid? https://doc.rust-lang.org/std/any/struct.TypeId.html

Undortunately not :/ See : https://users.rust-lang.org/t/get-type-information-in-proc-macro/48909

CyberHoward commented 2 months ago

I don't think we should add impl Into for every type of argument. I suggest we add a match statement that only converts the arguments to impl Into<> for the types:

* Uint (all sizes)

* String

But not for

* Addr

* Custom structures

* booleans

This helps us keep the generated fns readable (as you can only see the generated code in your IDE).

Yes good idea ! We just need to keep in mind that because of how proc-macros work, we will not be able to accept such bounds on type aliases like :

pub type ChainId = String;

Think that's fine. We could add a field attribute (like payable) that enables the Into manually as well.

Maybe we should start prefixing our macro attributes as well, like serde?

#[cw_orch(payable)]

#[cw_orch(into)]
Buckram123 commented 2 months ago

Maybe we should start prefixing our macro attributes as well, like serde?

#[cw_orch(payable)]

#[cw_orch(into)]

Pretty sure we should if we use such common phrases as into haha

Kayanski commented 2 months ago

Maybe we should start prefixing our macro attributes as well, like serde?

#[cw_orch(payable)]

#[cw_orch(into)]

Pretty sure we should if we use such common phrases as into haha

Yes there is a PR with that here already : https://github.com/AbstractSDK/cw-orchestrator/pull/408