-
Notifications
You must be signed in to change notification settings - Fork 355
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
cw1-whitelist-ng: Contract implementation in terms of semantical structures #499
Conversation
Couple comments from my side as an introduction (I feel as it is neccessary for +2k lines PR). First of all there are some files which can be probably ignored - NOTICE, README.md, schemas are taken straight from cw1-whitelist contract. Also I strongly encourage to make review commit by commit - no much changes happened in the process, additions only (besides some little alignments), and they are just incremental upgrades. Now about implementation. It is obvious there is a lot, even more than in default implementation. However most of this code is to be generated by the attribute macro. Therefor things which would land in the final contract, would be:
Now a word about new things. There are two new files to be generated:
In terms of tests itself - don't expect changes there. Only alignments to slight API changes (like calling functions on object instead of global functions). Whole logic is exactly the same. |
6b0684f
to
5aee758
Compare
Time to remove all schemas!!! @maurolacy was excited by this one :) |
Starting review now... I'm excited 😁 |
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.
Nice work. It looks clean and usage looks simple (in the tests).
I added a number of comments on how to clean up a few points here and there but the general design looks good.
I am curious how you plan to auto-generate the Msgs, but looking forward to those surprises as well.
admins: Vec<String>, | ||
) -> Result<Response<T>, Self::Error>; | ||
|
||
fn admin_list(&self, deps: Deps, env: Env) -> Result<AdminListResponse, Self::Error>; |
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.
Okay, so one trait for both execution and queries.
Curious if this will need to be separated later on when we mock out / customize, but happy to start with this.
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 think it would be one trait for both queries and exec, separation would be on the other level - one trait per API. Therefor this one would probably break into:
pub trait Cw1 {
fn execute(...) -> ...;
fn can_execute(...) -> ...;
}
pub Whitelist {
fn freeze(...) -> ...;
fn update_admins(...) -> ...;
fn admin_list(...) -> ...;
}
The Cw1
would be eventually extracted to packages/cw1
.
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.
That would be cool to see, even in the same package. Just to show how extension works.
pub trait Whitelist: Cw1 {}
is the same pattern we would need for one contract extending another one, same as multiple with the same interface, etc. We get the issue how how to dispatch messages.
Cw1Message and Cw1Query that can be dispatched to Cw1 trait, and somehow embed that in a WhitelistMsg that can dispatch to either Whitelist or Cw1... not sure if I am clear. Anyway, doing some (simple) trait extension will open up a large design space that is worthwhile to explore
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 completely don't see a reason why Whitelist
would have any relation to Cw1
. I can have whitelist restricting any other contract, those are two separate ideas.
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.
Fair enough. No need to have the traits extend each other
Let's get it to work now as one trait.
Having it as two separate traits and showing how the messages work with that would be a great demo to show composition. This can be a future step, just see how you can demo that even in one contract.
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 think having different traits for execution and queries would still be good, by example for auto-generation.
How do you plan to mark / detect queries from execution? Using macro attributes? Isn't there another / a better option?
Thanks for the responses. |
Another iteration, mostly addressing @ethanfrey comments. Most important changes:
|
b7ca929
to
c0f8aea
Compare
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.
Good stuff.
The only open comments are minor details, please merge this and we can address those with a much smaller diff.
- how parse errors are displayed in enrtry_execute when failing to match on all enums.
- naming of how to pass contract args to the instantiator in multitest
Like I said, minor cosmetics. But this design is awesome! And I can imagine much of this disappearing into macro-land.
Last comment - to not confuse other people, maybe we move this to contracts-ng/cw1-whitelist-ng
or something... so the different styles of contracts are not next to each other? Not now, but maybe in a future PR... this is mainly me trying to think how to support two versions of the same logic in different formats without confusing the consumers.
where | ||
T: DeserializeOwned, | ||
{ | ||
let mut errs = vec![]; |
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.
Interesting....
So it tries to parse into each enum and returns all the error messages appended.
I would be curious to see a test that asserted the error string when some garbage like {"foo": "bar"}
was passed in. (I know, asserting error strings is bad, maybe doc test?? something to show this doesn't look too terrible).
Minor point - can be a follow up PR.
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 am struggling here. Before it was done by just dumping as an error the error message (not even ParseErr
was constructed). I would actually prefer to return error looking like:
{
"parse_err": {
"Cw1ExecMsg": "...",
"WhitelistExecMsg": "...",
}
}
This actually can be done by providing additional custom error type which is either DispatchErr(HashMap<String, StdError>)
(this very case) or ContractErr(Self::Error)
(for failing in contract after dispatching succeed. For now I think this would be the best approach, but didn't want to put it here yet as I am polishing this idea.
{ | ||
let mut errs = vec![]; | ||
|
||
match from_slice::<Cw1ExecMsg<T>>(msg) { |
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 multiple attempts to deserialize is a nice approach. It looks a little ugly here, but I see the beauty of each message being able to implement dispatch and the contract being able to implement multiple traits.
If this is auto-generated (which it will be shortly), then I definitely see the bonus.
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 don't like this approach of "trying until succeeding", at all. It's inefficient, and unclear. It's also untyped... you're introducing a lot of types / encapsulation, and that's breaking badly here.
Isn't there a way to annotate the type a msg belongs to, and / or to use (a series of) typed handlers / pre-dispatchers?
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.
It is as efficient as it can be - and basically what serde does. It is what all parsers do basically - they check which pattern mach and choose it, and if at some point parsing stucks - it performs fallback to last place where it had another options. There is no other way as it to decide if message is Cw1ExecMsg
other than try to parse it. And my contract reacts to more than one type of message - I need to try work on both of those.
Actually I encourage you to generate the serde::Deserialize
for WhitelistExecMsg
(as it has more than one variant). I assume you expect something like:
match variant(token) {
"freeze" => deserialize_freeze(...),
"update_admins" => deserialize_update_admins(...),
_ => raise_error(...)
}
But what you actually get is:
if is_freeze_variant(token) {
return deserialize_freeze(...);
}
if is_update_admins(token) {
return deserialize_update_admins(...);
}
raise_error(...)
Except it is way more obscured (and this is the reason why serde is not the fastest json parser for rust btw - it is the most versatile). Any way my point is - it is state of the art for the parsers to try paths unless you find one (you can call it dynamic programming, you can call it DFS graph search - it is whole the same) as it is the most generic way to approach this. I could actually use the additional final structure and make it untagged probably: https://serde.rs/enum-representations.html#untagged - I will explore this option, but it is exactly what you would get at the end (except hidden in serde generated code).
I completely do not understand how is it untyped? I am not using any type conversions or so, I take what I got from stream and try to parse it into every path I can handle, but every try is fully typed. i completely do not understand the "Isn't there a way to annotate the type a msg belongs to, and / or to use (a series of) typed handlers / pre-dispatchers?" - all those messages belongs to many contracts. I can have arbitrary number of contracts reacting on Cw1ExecMsg
- all of them would be annotated with proper attribute, so dispatcher would be generated.
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 explored it to figure out why I didn't use untagged - it is not working with serde_json_wasm
. It is because it is relying on deserialize_any
, which is not implemented (which is a shame, and as @ethanfrey is owning it I think we could add it as it is done in normal non-wasm implementation to be fully compliant with it). But at the end it is exaclty what happens in non-wasm implementation
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 completely do not understand how is it untyped?
I mean, in the original version we get these messages already deserialized for us.
So, what you are saying is that the "trial and error" is being hidden from us during pre-processing? And that that's why I like it more? I'm not so sure, but, it may be. Perhaps implementing and using deserialize_any
is the way to go here then. To try and hide this "ugliness".
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.
Can't you instruct serde to parse to a family (or series) of types? That way, intermediate / partial results of the parsing can be re-utilized, instead of starting from scratch every time.
I grant you that I don't know how serde (or any other similar parser) works internally when deserializing, but it stretches my mind to think that it doesn't try to optimize / re-utilize intermediate / partial results somehow.
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.
Given the existing state of serde-json-wasm, and our dependence on it (which is a much bigger question and out of scope here), there is no better way.
CosmWasm/serde-json-wasm#20 proposed #[serde(flatten)]
, which would produce a nice way to make embedded enums. Here was an attempt, but again, required serde-json, which doesn't compile to valid wasm (imports floats): #84
I would keep this code as is, and potentially make a macro to auto-generate this, or use #[serde(flatten)]
after implementing that upstream.
I would envision something like:
pub enum ExecuteMsg {
#[serde(flatten)]
Cw1(Cw1ExecuteMsg),
#[serde(flatten)]
Whitelist(WhitelistExecuteMsg),
}
or
#[flatten_nested_enums]
pub enum ExecuteMsg {
Cw1(Cw1ExecuteMsg),
Whitelist(WhitelistExecuteMsg),
}
which would only handle the case for enums, where all variants are enums, and likely be much easier to implement as a custom macro.
|
||
assert_eq!( | ||
res.messages, | ||
msgs.into_iter().map(SubMsg::new).collect::<Vec<_>>() |
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 chain could make a nice helper function (in cosmwasm-std or cw-plus).
Or not... just a thought
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 could. But it is very out of scope of this one ;)
} | ||
|
||
#[test] | ||
fn execute_custom_messages_works() { |
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.
Thanks for adding this test to show T is properly supported
use cosmwasm_std::{CosmosMsg, Deps, DepsMut, Env, MessageInfo, Response}; | ||
use cw1::query::CanExecuteResponse; | ||
|
||
pub trait Cw1<T> { |
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.
Great to show support for multiple interfaces.
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 like this. If I'm not mistaken, this is the core of your idea. It's a great idea for re-usability and composition.
use crate::msg::{AdminListResponse, Cw1QueryMsg, WhitelistQueryMsg}; | ||
|
||
#[must_use] | ||
pub struct Cw1Querier<'a, C> |
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.
Nice implementation of Queriers. Look simpler and cleaner.
|
||
pub fn execute<C>(self, msgs: Vec<CosmosMsg<C>>) -> AnyResult<AppResponse> | ||
where | ||
C: Clone + std::fmt::Debug + PartialEq + JsonSchema + Serialize + 'static, |
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 should be CustomMsg
type now (maybe we need to update in cw-plus now this has landed in cosmwasm-std)
Nice use of late binding of where clause.
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 missed its existence there, will change.
} | ||
|
||
impl<'a, App> WhitelistExecutor<'a, App> { | ||
pub fn new(addr: Addr, app: &'a mut App, sender: Addr, send_funds: &'a [Coin]) -> Self { |
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 nice pattern. Always has the same XyzExecutor.new()
args, then contract-specific function call.
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.
You should not even call executor directly. You should have your Proxy
after instantiation and you call:
contract.whitelist_exec(...).update_admins(...).unwrap();
self | ||
} | ||
|
||
pub fn with<C>(self, admins: Vec<String>, mutable: bool) -> AnyResult<Cw1WhitelistProxy> |
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 don't like this name. The other optional with_{admin, label}
methods are great.
This should have a more different name. Maybe do
? I'm sure you can come up with something better...
let contract = Instantiator::new(....).with(...).unwrap();
looks a bit weird to me,.
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 don't like do
to much. I see your point, I somehow agree. Maybe call
? perform
? with_args
? I am struggling. Maybe someone have an idea?
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.
perform
? with_args
?
Both seem fine with me. Again, let's merge and discuss that in a separate thread
self.0.clone() | ||
} | ||
|
||
pub fn instantiate<'a, App>( |
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.
Hmmm... so I would really call it.
let contract = Cw1WhitelistProxy::instantiate(...).with(...).unwrap();
That makes more sense, but still... I could imagine a better name.
Not going to block merge with it, but happy for more naming feedback before this name gets ported to tons of auto-generated code.
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.
Agree
@ethanfrey thanks for aproval, I am stalling a little with merging, as I would like give a chance for others to comment (in particular maybe some better idea about naming). |
Or as I am thinking about it... there are no so bad things there, so I am merging to not asking for reaproval of this monster on every rebase. Tomorrow morning I would make a PR with minor changes (in particular introduce |
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.
Looks good! Just some comments / questions on naming and types. Will continue viewing it tomorrow.
pub mod state; | ||
|
||
#[cfg(not(feature = "library"))] | ||
mod binary { |
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 name does not make sense to me. Perhaps wrapper
or entry_points
?
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.
It is made as it is for "binary" release. entry_points
is good to. I would actually get rid of it at all on auto-gen code, I did it for saving on cfg
attributes - everything is anyway extracted to lib namespace below.
use cosmwasm_std::{CosmosMsg, Deps, DepsMut, Env, MessageInfo, Response}; | ||
use cw1::query::CanExecuteResponse; | ||
|
||
pub trait Cw1<T> { |
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 like this. If I'm not mistaken, this is the core of your idea. It's a great idea for re-usability and composition.
|
||
impl Cw1WhitelistContract<Empty> { | ||
// Native form of this contract as it is to be created in entry points | ||
pub const fn native() -> Self { |
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.
Is native
a proper name here? I understand the idea, that without custom messages you are basically "stuck" in the native chain, but still, it's a little confusing. Perhaps stock
or base
are better names?
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 don't know, it is just native type for this contract. I don't see how it is base
as it is not extensible in any way (it is actually the less extensible form), it is only to build entry points. It exists basically only to make it easier to construct it in MT/UT and I actually like the naming here. stock
may also work, but I have no recall seeing it anywhere in similar context and I like to reuse common naming so it is easier to get used to it.
admins: Vec<String>, | ||
) -> Result<Response<T>, Self::Error>; | ||
|
||
fn admin_list(&self, deps: Deps, env: Env) -> Result<AdminListResponse, Self::Error>; |
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 think having different traits for execution and queries would still be good, by example for auto-generation.
How do you plan to mark / detect queries from execution? Using macro attributes? Isn't there another / a better option?
@maurolacy - yes, by the attributes. I actually started some implementation some time ago (I will copy and continue work on it soon), an example Cw1 interface would look like this: #[cw_derive::interface]
trait Cw1<#[custom] T> {
type Error;
#[msg(exec)]
fn execute(&self, ctx: ExecCtx, msgs: Vec<CosmosMsg<T>>) -> Result<Response, Self::Error>;
#[msg(query)]
fn update_members(&self, ctx: QueryCtx, sender: String, msg: CosmosMsg<T>) -> Result<Response, Self::Error>;
} You see the attribute Splitting interfaces between query and execution is just wrong, because of their semantics. You never want to implement just one of them - if contract implements Actually if I would like to be like really fancy I could distinguish the execs from queries by their arguments (one take |
I for one prefer the explicit arguments (the tuple itself) , and don't like the
Makes sense.
It will also be more brittle / obscure. I think macro attributes are good, both in terms of clarity / flexibility, and simplicity. Besides, it's still too early for our own CosmWasm SmartContracts™️ DSL I guess. ;-) |
OK, did a quick comparison of "classic" vs. "ng" (optimized) contracts (didn't check, but I assume functionality is currently equivalent):
"ng" contracts are ~15% larger than "classic" ones. Not a big deal, but let's keep an eye on this. |
@maurolacy - good point. It would be nice to check how they differ in terms of gas usage. And also it is interesting what makes them actually bigger - I actually think, that it is possible that the helpers like |
Created #505 for it. |
Closes #494