Skip to content

Conversation

@Kayanski
Copy link
Contributor

@Kayanski Kayanski commented May 17, 2024

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 #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 #398

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 17, 2024

Deploying cw-orchestrator with  Cloudflare Pages  Cloudflare Pages

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

View logs

@Kayanski Kayanski added the breaking This change prompts a major version bump. label May 17, 2024
@Kayanski Kayanski requested review from Buckram123 and CyberHoward and removed request for CyberHoward June 3, 2024 09:57
Copy link
Contributor

@Buckram123 Buckram123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, shouldn't it require minor bump instead of patch for cw-orch-fns-derive? I guess missing

  • Changelog update
  • Docs update where impl_into mentioned

Not sure about the file that I skipped, looked like cargo expand output, lmk if I should check it as well

Copy link
Contributor

@CyberHoward CyberHoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Kayanski Kayanski changed the base branch from main to update/merge-ex-qu-derive-fns June 4, 2024 08:02
@Kayanski
Copy link
Contributor Author

Kayanski commented Jun 4, 2024

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;

Base automatically changed from update/merge-ex-qu-derive-fns to main June 4, 2024 08:15
@codecov
Copy link

codecov bot commented Jun 4, 2024

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.

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 Coverage Δ
contracts/mock_contract/src/lib.rs 97.9% <100.0%> (ø)
contracts/mock_contract/src/msg_tests.rs 94.7% <100.0%> (ø)
contracts/mock_contract_u64/src/lib.rs 82.7% <100.0%> (-0.4%) ⬇️
...ckages/macros/cw-orch-fns-derive/src/fns_derive.rs 100.0% <100.0%> (ø)
packages/macros/cw-orch-fns-derive/src/lib.rs 100.0% <ø> (ø)
packages/macros/cw-orch-fns-derive/src/helpers.rs 78.7% <96.1%> (+5.5%) ⬆️

... and 17 files with indirect coverage changes

@Buckram123
Copy link
Contributor

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
Copy link
Contributor Author

Kayanski commented Jun 4, 2024

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
Copy link
Contributor

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
Copy link
Contributor

Buckram123 commented Jun 4, 2024

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
Copy link
Contributor Author

Kayanski commented Jun 4, 2024

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 : #408

@Kayanski Kayanski merged commit 7d28f5b into main Jun 7, 2024
@Kayanski Kayanski deleted the nicolas/orc-73-simplify-contract-interface branch June 7, 2024 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking This change prompts a major version bump.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants