Add Sylvia helpers #21
Conversation
|
Does this supersede #20? |
There was a problem hiding this comment.
The same comment as #20, except I see more helpers which are not yet generated in Sylvia so it would make sense for longer. Queriers are ongoing, and will probably be released next week; generating exec messages is next in the queue.
I will accept that.
Usage of generated queries would be a touch different - instead of passing deps to the calls, the temporary querier object would be generated (with the querier function on generated helper object), so it doesn't need to be passed with every single query (but there still be a type to be stored as remote contract address).
However this is good as it is, and will allow progress, and when we release queries I will just do alignment of all usages quickly.
| impl Deref for CrossStakingApiHelper { | ||
| type Target = Addr; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| &self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
I would strongly avoid implementing Deref trait like this - long discussion why, but long story short: even Rust documentation claims:
On the other hand, the rules regarding Deref and DerefMut were designed specifically to accommodate smart pointers. Because of this, Deref should only be implemented for smart pointers to avoid confusion.
Instead, I would implement AsRef<Addr> on this type, and possibly directly the pub fn addr(&self) -> &Addr function.
There was a problem hiding this comment.
Okay. I just used Deref for things like struct Addr(String), where it let's us use all the wrapped type's functions without manually passing them through. Fair enough to remove it and force this explicit.
And happy if you give me some Rust coaching on these language semantics.
| owner: String, | ||
| amount: Uint128, | ||
| msg: Binary, | ||
| funds: Vec<Coin>, |
There was a problem hiding this comment.
You are mixing message-related arguments with "metadata" (here: funds). Some messages would need those funds with them, some not, but what is a bit confusing - it is not clear if funds are funds sent with message or the argument of the message.
Here it is, I think, ok, but to make it clear in Sylvia, we would go with more like a builder pattern, so message arguments and their metadata would be passed separately.
There was a problem hiding this comment.
It is to make it short and standard. Happy if you:
(a) make a PR to clean up this concrete API
(b) use that cleaner concrete implementation as a basis for Sylvia
In fact, Sylvia should generation the exact same code as your refactored helpers. Then we can swap them out with a Sylvia upgrade and some code deletion.
I will remove Deref and then merge this.
Nice if you make a PR to change these, separate from the other PRs for writing contracts (base those contracts on your helper refactor PR). And then show your cleaned up version to Jan to implement with proc_macro.
Add some helper classes to call
#[interface]traits (struct that wraps execute and query methods to use in cross-contract calls).This is written by hand now to simplify the coding in the contracts (which will be repeated in mocks and real contracts). But ideally this could be autogenerated in a future sylvia release.
I added
funds: Vec<Coin>as an extra arg to all execute methods (even ones that don't need funds) to simulate the API that would be generated by sylvia codegen. But it is already usable as is.