-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Demo: add cw-orch support #375
Demo: add cw-orch support #375
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments on the state of this POC.
How it works, and what further work would be needed.
I'd love responses on these points
@@ -8,6 +8,7 @@ version = "0.10.1" | |||
|
|||
[workspace.dependencies] | |||
sylvia-derive = { version = "0.10.1", path = "sylvia-derive" } | |||
cw-orch = { git = "https://github.com/abstractsdk/cw-orchestrator", branch = "update/relax-bound-on-derive" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously, this would need to be merged and tagged before we can accept a PR, but that should be doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bigger question is why we have to tie Sylvia (even as an optional dependency) to one version of cw-orch. (eg 0.10x sylvia uses cw-orch 0.22, 0.11.x sylvia uses cw-orch 0.23). Maybe someone with more macro magic than I can see how to solve that.
// Maybe uploadable can be autogenerated? | ||
// But this is fine to include in the client code | ||
|
||
#[cw_orch::interface(InstantiateMsg, ContractExecMsg, ContractQueryMsg, Empty)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can try to auto-generate this, or just leave this done by hand in the contracts that enable the "orch" feature
// #[sv::messages(cw20_marketing as Marketing)] | ||
// #[sv::messages(cw20_minting as Minting)] | ||
|
||
impl From<cw20_allowances::sv::Cw20AllowancesExecMsg> for ContractExecMsg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally Sylvia could generate these for all the interfaces that are used as ContractExecMsg
variants. (Help wanted... the commit by @Kayanski shows the limit of trait associated types.)
None, | ||
)?; | ||
|
||
let balance = contract.balance(mock.sender().to_string())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the contract with nice syntax.
That works on multi-test, testnet, etc.
👍
let balance = contract.balance(mock.sender().to_string())?; | ||
assert_eq!(balance.balance.u128(), 150_000u128); | ||
|
||
// This is what fails... let's see how to make that happen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old comment, I fixed this... that is why the hand-rolled "From" statements are needed. Can we agree the new form is cleaner?
@@ -16,6 +16,7 @@ cosmwasm-std = { workspace = true, features = ["staking"] } | |||
cosmwasm-schema = { workspace = true } | |||
serde = { workspace = true } | |||
sylvia = { path = "../../../sylvia" } | |||
cw-orch = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a trick.
All the interfaces (or actually any sylvia crate) that will be imported by a cw-orch
-enabled sylvia contract will need to have this dependency. No code changes, just the dependency. The macro exansion enabled by the orch
feature ends up referencing ::cw_orch::...
. Maybe there is a way around it?
Only seems a problem if you import interface crates that are not in your repo
@@ -354,8 +354,19 @@ impl<'a> ContractEnumMessage<'a> { | |||
let ctx_type = msg_ty.emit_ctx_type(&custom.query_or_default()); | |||
let ret_type = msg_ty.emit_result_type(&custom.msg_or_default(), &error.error); | |||
|
|||
#[cfg(feature = "orch")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change #1: add derives to the messages
|
||
// Append this derive for cw-orch contracts | ||
// (Optionally you could always do this?) | ||
#[cfg(feature = "orch")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change #2: Add the From statement for the contract specific message.
(Note: we want to extend this for all the messages from all the interface variants, but that proved hard)
@@ -161,4 +162,20 @@ impl MsgType { | |||
}, | |||
} | |||
} | |||
|
|||
#[cfg(feature = "orch")] | |||
pub fn emit_derive_call(&self) -> TokenStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change #3: add those derive bounds on other messages
@@ -27,3 +27,5 @@ pub use { | |||
cosmwasm_schema as cw_schema, cosmwasm_std as cw_std, schemars, serde, | |||
serde_cw_value as serde_value, serde_json_wasm as serde_json, | |||
}; | |||
#[cfg(feature = "orch")] | |||
pub use cw_orch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditionally export cw-orch here so that those lines like #sylvia:: cw_orch::QueryFns
compile
@@ -27,3 +27,5 @@ pub use { | |||
cosmwasm_schema as cw_schema, cosmwasm_std as cw_std, schemars, serde, | |||
serde_cw_value as serde_value, serde_json_wasm as serde_json, | |||
}; | |||
#[cfg(feature = "orch")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make it visible in docs as reexported if cw_orch
is enabled.
#[cfg(feature = "orch")] | |
#[cfg_attr(docsrs, doc(cfg(feature = "cw_orch")))] | |
#[cfg(feature = "orch")] |
|
||
#[cfg(feature = "orch")] | ||
pub fn emit_derive_call(&self) -> TokenStream { | ||
let sylvia = crate_module(); | ||
match self { | ||
MsgType::Query => quote! { | ||
#[derive(#sylvia ::serde::Serialize, #sylvia ::serde::Deserialize, Clone, Debug, PartialEq, #sylvia ::schemars::JsonSchema, #sylvia:: cw_schema::QueryResponses, #sylvia:: cw_orch::QueryFns)] | ||
}, | ||
MsgType::Exec => quote! { | ||
#[derive(#sylvia ::serde::Serialize, #sylvia ::serde::Deserialize, Clone, Debug, PartialEq, #sylvia ::schemars::JsonSchema, #sylvia:: cw_orch::ExecuteFns)] | ||
}, | ||
_ => quote! { | ||
#[derive(#sylvia ::serde::Serialize, #sylvia ::serde::Deserialize, Clone, Debug, PartialEq, #sylvia ::schemars::JsonSchema)] | ||
}, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for such split.
Add method in MsgType
to return #sylvia:: cw_orch::QueryFns
or #sylvia:: cw_orch::ExecuteFns
for MsgType::Query/Exec
respectively if flag is enabled.
MsgType::Query => { | ||
quote! { #sylvia ::cw_schema::QueryResponses, #sylvia:: cw_orch::QueryFns } | ||
} | ||
MsgType::Exec => quote! { #sylvia:: cw_orch::ExecuteFns }, | ||
_ => quote! {}, | ||
}; | ||
#[cfg(not(feature = "orch"))] | ||
let derive_query = match msg_ty { | ||
MsgType::Query => { | ||
quote! { #sylvia ::cw_schema::QueryResponses } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in case of emit_derive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the need and generalized it in #379 issue that should solve the problem.
The big issue I have here is binding the Sylvia implementation to particular external library. I am a fan of providing the API to integrate them, but I really don't like the idea of API saying "you can use this feature, but then you need to add additional dependency and it has to be in particular version". This adds a lot of potential maintenance work - because, at some point, some bug might occur, saying, "I use Sylvia x.y.z with orchestrator a.b.c, and it doesn't work.".
Providing a way to simply forward attributes to messages doesn't create artificial dependency corelations.
I support this approach 100% and was struggling to remove the import, which I couldn't (and signalled as an issue). I have no idea how one can extend remote macros. But if there is a way, and it can cover the code points here, it would be great. And this serves as an example of what places would need to be extended. Also if #376 is implement, the only points left to extend are just adding some derive macros on top of the Execute, Query, Sudo messages, which should be a reasonable target. And a lot more clear than "allow others to extend". Very happy to close this PR and use another approach if it can acheive this outcome |
This is a great point that we identified a while ago. To solve this we've stabilized the core traits and types used in cw-orch. See the cw-orch-core crate. This change essentially means that an interface with cw-orch v0.20.x is compatible with a version of v0.21.x and above until the next breaking change to core (which should be rare). This means that not all crates have to be on the same version of cw-orch. Lmk if you think we're missing something with this or if there's a better way for us to make this clear to the crate consumer! Edit: will spend some time on this PR this weekend :) |
Hi Ethan, Would you agree to close this demo PR? We already have two issues that specify the problem demonstrated in this PR:
The first of these is already addressed by #388. |
@ethanfrey, did you check if the attributes forwarding satisfy the cw-orch support? |
This is a demo PR to show the possibility of implementing #374
It has some short-comings, but in the current state, it allows crates to optionally enable cw_orch derives in sylvia and use it in their crate. While leaving crates that don't opt-in with the same code-gen as before (meaning it is potentially non-API breaking).
In this case, @Kayanski modified the cw20-base sylvia example to play well with the cw-orch framework and we run a multi-test on the cw-orch generated code to show it works.
I will highlight some limitations of the current PR that need to be fixed before such could be merged, but this should provide a concrete example to move any potential cw-orch and sylvia collaboration forward