CW precompile support with Schema, updated precompile for DEX with full coverage #3722

dzmitry-lahoda · 2023-06-21T14:48:41Z

I have generated Schema for precompiles so these can be used to generate Rust again by CW people.

DEX is fully covered by CW too, and also improved coding of precompiles.

Cleaned up some garbage and useless comments.

Main goal is to provide interface to CW people.

I will test this precompile when will reach it with multichain XCVM swap.

PR title is my best effort to provide summary of changes and has clear text to be part of release notes
I marked PR by misc label if it should not be in release notes
I have linked Zenhub/Github or any other reference item if one exists
I was clear on what type of deployment required to release my changes (node, runtime, contract, indexer, on chain operation, frontend, infrastructure) if any in PR title or description
I waited and did best effort for pr-workflow-check / draft-release-check to finish with success(green check mark) with my changes
I have added at least one reviewer in reviewers list
I tagged(@) or used other form of notification of one person who I think can handle best review of this PR
I have proved that PR has no general regressions of relevant features and processes required to release into production

mina86

Roughly speaking LGTM, but I need to have another pass to look closely at the actual logic.

mina86 · 2023-06-22T16:42:32Z

code/parachain/frame/composable-traits/src/lib.rs

@@ -51,6 +50,7 @@ pub mod governance;
 pub mod lending;
 pub mod liquidation;
 pub mod oracle;
+pub mod prelude;


Do we actually need it? Especially since this exports symbols from other crates which I’d argue shouldn’t be in prelodue. To be honest in general I think preludes should not exist in the first place.

i need.

you say that all preludes include only this crate items? all the time and this is default behavior of all preludes?

parity has preludes for pallets and for core. rust team provides prelude for std.

why you you think such?

Well, preludes can always be replaced by importing specific symbols so are never really needed. I also cannot comment what all preludes do; I’m sure there are some which include symbols from various unrelated crates, however yes, I would argue that reexporting things from bunch of crates is somewhat surprising.

The problem with preludes, or more specifically with wildcard imports, is that it makes the code harder to read for someone who is unfamiliar with it. If I saw MaxEncodedLen or TypeInfo in the code I would have no idea what those types are and where to look for their documentation.

This is also true about Rust’s preludes and I’m not convinced they are good addition to the standard library, but at least there the prelude includes symbols which are known to anyone who knows basics of Rust.

MaxEncodedLen - it allows to bound unbounded types like Vec or String. Parity likes bounds in storage and on the wire.

TypeInfo - it is like JsonSchema, but specifically for scale encoder.

Each prelude include layer of new known by default preludes. Like if you are Substrate or CW integration (String/Vec/Json) you have you own well known layer. 100% preludes include what every knows in Substrate or CW or XCM, depending on what layer you are.

I shared examples of parity preludes, so in this PR not 100% same, but kind of similar.

Specific imports can always be replaced by preludes.

code/parachain/frame/composable-traits/src/dex.rs

code/parachain/frame/cosmwasm/src/lib.rs

code/parachain/frame/cosmwasm/src/runtimes/vm.rs

code/parachain/runtime/common/src/cosmwasm.rs

code/parachain/runtime/common/src/dex.rs

rfcs/0008-pablo-lbp-cpp-restructure.md

code/parachain/frame/composable-traits/schema/composable-traits.json

mina86 · 2023-06-22T18:29:31Z

code/parachain/runtime/common/src/dex.rs

+
+	pub fn query(sender: &str, msg: QueryMsg) -> Result<QueryResponse, CosmwasmPrecompileError> {
+		match msg {
+			QueryMsg::Assets { pool_id } => {


Could you extract each arm to a separate method, something like:

pub fn query(sender: &str, msg: QueryMsg) -> Result<QueryResponse, CosmwasmPrecompileError> { match msg { QueryMsg::Assets { pool_id } => self.handle_query_assets(sender, pool_id), QueryMsg::SpotPrice { pool_id, base_asset, quote_asset_id, calculate_with_fees } => { handle_query_spot_price(sender, pool_id, base_asset, quote_asset_id, calculate_with_fees) }, /* ... */

and same with execute? The long functions and deep indentation
levels make the code less readable than it could be.

handle_query_spot_price will require generics and will make more code.

and you are wrong, consider QueryMsg::SpotPrice to be inline function. and block => { } to be its body.

Adding function is just noise. So your solution is opinionted, adds more noise, more code, harder to read.

Why would it require generics? It would be no more generic than the query method.

It adds noise because you have to define separate methods with arguments, but the point is that you end up with much shorter functions with cleaner control flow. The functions can also be properly documented with a comment.

control flow is not cleaner. match = functions.

functions are documented on query types. query case = function. match block = function.

generic would be required on function declaration.

control flow is not cleaner. match = functions.

I don’t know what you mean by ‘match = functions’.

functions are documented on query types.

Except the types aren’t documented either.

generic would be required on function declaration.

No, it would be method of Dex and as such would not need any additional generic arguments.

each variant in CW enum is functions. So when you match, you do functions. And each functions has name (in match variant) and each block of match if function body. So it is same thing encoded other way.

They documented as comments to CW people where needed.

mina86 · 2023-06-22T19:23:56Z

code/parachain/frame/composable-traits/src/lib.rs

@@ -51,6 +50,7 @@ pub mod governance;
 pub mod lending;
 pub mod liquidation;
 pub mod oracle;
+pub mod prelude;


Well, preludes can always be replaced by importing specific symbols so are never really needed. I also cannot comment what all preludes do; I’m sure there are some which include symbols from various unrelated crates, however yes, I would argue that reexporting things from bunch of crates is somewhat surprising.

The problem with preludes, or more specifically with wildcard imports, is that it makes the code harder to read for someone who is unfamiliar with it. If I saw MaxEncodedLen or TypeInfo in the code I would have no idea what those types are and where to look for their documentation.

This is also true about Rust’s preludes and I’m not convinced they are good addition to the standard library, but at least there the prelude includes symbols which are known to anyone who knows basics of Rust.

code/parachain/runtime/common/src/dex.rs

mina86 · 2023-06-22T21:06:13Z

code/parachain/runtime/common/src/dex.rs

+
+	pub fn query(sender: &str, msg: QueryMsg) -> Result<QueryResponse, CosmwasmPrecompileError> {
+		match msg {
+			QueryMsg::Assets { pool_id } => {


Why would it require generics? It would be no more generic than the query method.

It adds noise because you have to define separate methods with arguments, but the point is that you end up with much shorter functions with cleaner control flow. The functions can also be properly documented with a comment.

code/parachain/frame/composable-traits/src/dex.rs

mina86

LGTM sans the comments.

code/parachain/runtime/common/src/dex.rs

code/parachain/runtime/picasso/src/contracts.rs

dzmitry-lahoda · 2023-06-23T15:40:32Z

@mina86

Part of writing the code is documenting the code so yes, it is your task. The code needs to be commented not only for downstream users but also for people who review the code (like me) or the next person who might need to maintain it. If having no comments is accepted by the company than I really have issues with that stance.

please write RFC for that or setup meeting with Blas and Parachain lead, OR take lead on collaboration with CW people.

dzmitry-lahoda · 2023-06-23T15:43:47Z

@mina86

It adds noise because you have to define separate methods with arguments, but the point is that you end up with much shorter functions with cleaner control flow

its lie, I do not get shorted functions, i just got harder to read code, more code and more noise and more copy paste.

mina86 · 2023-06-23T16:29:38Z

Part of writing the code is documenting the code so yes, it is your task. The code needs to be commented not only for downstream users but also for people who review the code (like me) or the next person who might need to maintain it. If having no comments is accepted by the company than I really have issues with that stance.

please write RFC for that or setup meeting with Blas and Parachain lead, OR take lead on collaboration with CW people.

@blasrodri, as per @dzmitry-lahoda’s request pinging you about this. The current state of parts of the codebase is subpar to put it mildly with many parts of the code completely undocumented. Seems to me we need to agree on some style guide which at minimum would enforce documenting new code.

dzmitry-lahoda · 2023-06-23T16:32:37Z

@mina86

Well, preludes can always be replaced by importing specific symbols so are never really needed. I also cannot comment what all preludes do; I’m sure there are some which include symbols from various unrelated crates, however yes, I would argue that reexporting things from bunch of crates is somewhat surprising.

The problem with preludes, or more specifically with wildcard imports, is that it makes the code harder to read for someone who is unfamiliar with it. If I saw MaxEncodedLen or TypeInfo in the code I would have no idea what those types are and where to look for their documentation.

This is also true about Rust’s preludes and I’m not convinced they are good addition to the standard library, but at least there the prelude includes symbols which are known to anyone who knows basics of Rust.

Let discuss preludes in Slack or on separate meeting, I used preludes a lot and Parity Uses.

See https://github.com/paritytech/substrate/blob/dc3fb3d4cf9efb779a068cdc201babc7ae2d233c/frame/support/src/lib.rs#L1566 . May be style of coding was good in NEAR, ok with that. By I am for style of coding of Parity as wwe code in Substrate.

Also you can see that most preludes are internal, so they will not surprise you if other crate, in this crate - you should be aware of crate. Also preludes are must for NO_STD and CW support - no other sane way.

dzmitry-lahoda · 2023-06-23T16:35:40Z

@blasrodri, as per @dzmitry-lahoda’s request pinging you about this. The current state of parts of the codebase is subpar to put it mildly with many parts of the code completely undocumented. Seems to me we need to agree on some style guide which at minimum would enforce documenting new code.

Let agree on next PRs so. If code requirement appeared after work was done and not critical to function, it is no part of this PR.

Oh, i had dozen of times these discussion, in the end I can say - if CI works with that PR - PR is good :) You may not believe me, but I have seen dozens of projects - all are same.

dzmitry-lahoda · 2023-06-23T16:39:00Z

@blasrodri @mina86 refers to CW query/execute interface, my requirements from CW people was.

API similar to Osmosis (I have documented that).
Rust code of API - done
Schema of API - done
I added - use latest CW-STD as it is best for documenting API - done.

CW people never raised requirements about docs to me.
5. On request for Pablo docs, our Biz people replied with all relevant info.

As for CW docs, if @mina86 wants nice CW sandbox for him to work - he can take time and CP paste docs and improve form what we have to CW and learn Substrate API.

I do not have time to hide that we have substrate API on docs level, I already hidden that on code level.

Also I always open for propositions to take lead on Precompiles and CW people support and do whatever is best :)

And any comprehensive non opinionated discussion for most objective way to get things.

mina86 · 2023-06-23T16:44:22Z

if CI works with that PR - PR is good

This is blatantly false. Code which is not readable and not maintainable isn’t good even if it works in the moment.

@mina86 refers to CW query/execute interface

No, I’m referring to code as a whole. My comment regarding need for documentation isn’t limited to any specific part of the code base. If you’re writing undocumented code only for a single group of people than don’t expect me to review or work with it.

And any comprehensive non opinionated discussion for most objective way to get things.

I’m sorry, but are you suggesting that wanting documentation is ‘opinionated’?

mina86

LGTM sans the comments.

Ah, forgot to approve.

dzmitry-lahoda · 2023-06-23T17:03:30Z

@mina86 my code is not readable and not maintainable? :(

dzmitry-lahoda · 2023-06-23T17:07:18Z

@mina86 i tell there is process and definion of documentation. i adhere to that on what we have in composable. did you know that docs are not only /// and any such /// has a price?

if you review and you are not clear on code, just ask in code comment. i will answer or add docs. so i expect people to read code with accepted level of docs and ask question. yes.

feel free. ot to review anything. i am ok with that too)

to be honest i do not expect people work at all, not only review. that is usual. feel free to work as you wish.

mina86 · 2023-06-24T13:11:23Z

@mina86 my code is not readable and not maintainable? :(

I didn’t say that. I said CI passing is insufficient to call a PR good.

did you know that docs are not only /// and any such /// has a price?

And lack of /// has a price as well.

dzmitry-lahoda · 2023-06-28T13:48:18Z

any more responses about this?

I responded to preludes and docs, kind of:

docs are enough for CW people - will improve upon request, any specific comments can be clarified per line as needed
preludes, there is such practices kind of in substrate/parity and compoble repo. so kind of it what is was here, I working in that frame. sure there are good and bad parts for these.

…ll coverage (#3722) I have generated Schema for precompiles so these can be used to generate Rust again by CW people. DEX is fully covered by CW too, and also improved coding of precompiles. Cleaned up some garbage and useless comments. Main goal is to provide interface to CW people. I will test this precompile when will reach it with multichain XCVM swap. - [x] PR title is my best effort to provide summary of changes and has clear text to be part of release notes - [x] I marked PR by `misc` label if it should not be in release notes - [x] I have linked Zenhub/Github or any other reference item if one exists - [x] I was clear on what type of deployment required to release my changes (node, runtime, contract, indexer, on chain operation, frontend, infrastructure) if any in PR title or description - [ ] I waited and did best effort for `pr-workflow-check / draft-release-check` to finish with success(green check mark) with my changes - [x] I have added at least one reviewer in reviewers list - [x] I tagged(@) or used other form of notification of one person who I think can handle best review of this PR - [x] I have proved that PR has no general regressions of relevant features and processes required to release into production

dzmitry-lahoda marked this pull request as draft June 21, 2023 14:48

dzmitry-lahoda changed the title ~~wrap wrap wrap~~ cw osmosis gamm dex precompile Jun 21, 2023

dzmitry-lahoda changed the title ~~cw osmosis gamm dex precompile~~ cw dex precompile updates Jun 21, 2023

dzmitry-lahoda changed the title ~~cw dex precompile updates~~ CW precompile support with Schema, updated precompile for DEX with full coverage Jun 22, 2023

dzmitry-lahoda requested review from mina86 and kkast and removed request for mina86 June 22, 2023 16:22

dzmitry-lahoda marked this pull request as ready for review June 22, 2023 16:22

dzmitry-lahoda enabled auto-merge June 22, 2023 16:27

mina86 reviewed Jun 22, 2023

View reviewed changes

mina86 reviewed Jun 23, 2023

View reviewed changes

code/parachain/runtime/common/src/dex.rs Outdated Show resolved Hide resolved

code/parachain/runtime/picasso/src/contracts.rs Outdated Show resolved Hide resolved

code/parachain/runtime/picasso/src/contracts.rs Outdated Show resolved Hide resolved

mina86 previously approved these changes Jun 23, 2023

View reviewed changes

dzmitry-lahoda dismissed mina86’s stale review via 6ecf087 June 23, 2023 17:00

dzmitry-lahoda added 6 commits June 23, 2023 21:46

wrap wrap wrap

848e52f

blunt

7c76578

cw sp

c9e4be4

compiles

0e121a3

account

f94dfae

finalized doc

402f5dc

dzmitry-lahoda added 13 commits June 23, 2023 21:47

primitives

4aa38ad

compiles

65c3908

std compatible

a860022

wasm support

2289dd2

common

f7b92fe

better query

2c7086e

hoho

c6a325a

done

0921b6e

schema generate

e117b11

common fixes

2c8b194

dex types

120af0f

fixed code

e3512dc

fmt

4a95dc5

dzmitry-lahoda force-pushed the dz/248 branch from 4332076 to 4a95dc5 Compare June 23, 2023 20:47

dzmitry-lahoda requested a review from mina86 June 23, 2023 20:47

fmt, clippy, tests

5add427

dzmitry-lahoda requested a review from RustNinja June 27, 2023 11:47

dzmitry-lahoda requested a review from blasrodri June 28, 2023 13:48

dzmitry-lahoda disabled auto-merge June 28, 2023 19:17

dzmitry-lahoda force-pushed the dz/248 branch from d1421d2 to 5add427 Compare June 28, 2023 19:24

dzmitry-lahoda merged commit bd77d68 into main Jun 28, 2023

dzmitry-lahoda deleted the dz/248 branch June 28, 2023 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CW precompile support with Schema, updated precompile for DEX with full coverage #3722

CW precompile support with Schema, updated precompile for DEX with full coverage #3722

dzmitry-lahoda commented Jun 21, 2023 •

edited

Loading

mina86 left a comment

mina86 Jun 22, 2023

dzmitry-lahoda Jun 22, 2023

mina86 Jun 22, 2023

dzmitry-lahoda Jun 27, 2023

dzmitry-lahoda Jun 27, 2023

dzmitry-lahoda Jun 27, 2023

dzmitry-lahoda Jun 27, 2023

mina86 Jun 22, 2023

dzmitry-lahoda Jun 22, 2023

mina86 Jun 22, 2023

dzmitry-lahoda Jun 23, 2023

dzmitry-lahoda Jun 23, 2023

dzmitry-lahoda Jun 23, 2023

mina86 Jun 23, 2023

dzmitry-lahoda Jun 27, 2023

dzmitry-lahoda Jun 27, 2023

mina86 Jun 22, 2023

mina86 Jun 22, 2023

mina86 left a comment

dzmitry-lahoda commented Jun 23, 2023 •

edited

Loading

dzmitry-lahoda commented Jun 23, 2023

mina86 commented Jun 23, 2023

dzmitry-lahoda commented Jun 23, 2023 •

edited

Loading

dzmitry-lahoda commented Jun 23, 2023

dzmitry-lahoda commented Jun 23, 2023 •

edited

Loading

mina86 commented Jun 23, 2023

mina86 left a comment

dzmitry-lahoda commented Jun 23, 2023

dzmitry-lahoda commented Jun 23, 2023 •

edited

Loading

mina86 commented Jun 24, 2023

dzmitry-lahoda commented Jun 28, 2023

CW precompile support with Schema, updated precompile for DEX with full coverage #3722

CW precompile support with Schema, updated precompile for DEX with full coverage #3722

Conversation

dzmitry-lahoda commented Jun 21, 2023 • edited Loading

mina86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mina86 left a comment

Choose a reason for hiding this comment

dzmitry-lahoda commented Jun 23, 2023 • edited Loading

dzmitry-lahoda commented Jun 23, 2023

mina86 commented Jun 23, 2023

dzmitry-lahoda commented Jun 23, 2023 • edited Loading

dzmitry-lahoda commented Jun 23, 2023

dzmitry-lahoda commented Jun 23, 2023 • edited Loading

mina86 commented Jun 23, 2023

mina86 left a comment

Choose a reason for hiding this comment

dzmitry-lahoda commented Jun 23, 2023

dzmitry-lahoda commented Jun 23, 2023 • edited Loading

mina86 commented Jun 24, 2023

dzmitry-lahoda commented Jun 28, 2023

dzmitry-lahoda commented Jun 21, 2023 •

edited

Loading

dzmitry-lahoda commented Jun 23, 2023 •

edited

Loading

dzmitry-lahoda commented Jun 23, 2023 •

edited

Loading

dzmitry-lahoda commented Jun 23, 2023 •

edited

Loading

dzmitry-lahoda commented Jun 23, 2023 •

edited

Loading