feat: x/cosmwasmpool proto and query boilerplate #4764

p0mvn · 2023-03-28T01:53:33Z

Closes: #XXX

What is the purpose of the change

Merging protos and query boilerplate from:
#4675

Note that there are no parameters or queries at this moment. However, we will likely need to add some. Since all wiring is already setup in #4675, I suggest we merge this as is and remove if unused at the end.

No actual logic changes or state breaks in this PR.

Brief Changelog

Copy proto definitions from feat/spike: CosmWasm Pool Type and Module #4675

make proto-all

make run-querygen

Copy minimum dependency imports and wiring. Namely,

params
keeper declaration and param methods
expected keeper interfaces in types
keys in types
pool store model in model package

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? no
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
How is the feature or change documented? not applicable

p0mvn · 2023-03-28T02:19:59Z

x/cosmwasmpool/types/params.go

+
+// Parameter store keys.
+var (
+// KeyParamField = []byte("TODO: CHANGE ME")


Note to reviewer: revisit having parameters is tracked in #4765

p0mvn · 2023-03-28T02:21:29Z

proto/osmosis/cosmwasmpool/v1beta1/tx.proto

+
+option go_package = "github.com/osmosis-labs/osmosis/v15/x/cosmwasmpool/types";
+
+service Msg {}


Note to reviewer revisit having messages is tracked in: #4765

p0mvn · 2023-03-28T02:22:33Z

x/cosmwasmpool/keeper.go

+	// TODO: remove nolint once added.
+	// nolint: unused
+	poolmanagerKeeper types.PoolManagerKeeper
+	// TODO: remove nolint once added.
+	// nolint: unused
+	contractKeeper types.ContractKeeper
+	// TODO: remove nolint once added.
+	// nolint: unused
+	wasmKeeper types.WasmKeeper


Note to reviewer: TODOs are tracked in: #4765

WasmKeeper allows for Create and Instantiate. Normally we pass a PermissionedKeeper so that it can't add new codes. I think it's ok, but if we want to be restrictive we can pass a PermissionedKeeper with custom permissions that don't allow Create

I see that this is defined in expected_keepers (I thought before it was wasm.WasmKeeper). We can definitely use a more restrictive one here. The ContractKeeper will need the permisions described above though

Yes, I'm defining the bare minimum of what interface is required by this module in types.expected_keepers.go

p0mvn · 2023-03-28T02:22:58Z

x/cosmwasmpool/model/msgs.go

+	// TODO: uncomment once merging state-breaks.
+	// return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&msg))


Note to reviewer: TODO is tracked in

p0mvn · 2023-03-28T02:23:10Z

x/cosmwasmpool/model/msgs.go

+	// TODO: uncomment once merging state-breaks.
+	// poolI := NewCosmWasmPool(poolID, msg.CodeId, msg.InstantiateMsg)
+	// return &poolI, nil


Note to reviewer: TODO is tracked in #4765

x/cosmwasmpool/types/expected_keepers.go

nicolaslara · 2023-03-28T12:50:26Z

proto/osmosis/cosmwasmpool/v1beta1/model/pool.proto

+  string contract_address = 2
+      [ (gogoproto.moretags) = "yaml:\"contract_address\"" ];
+  uint64 pool_id = 3;
+  uint64 code_id = 4;


Do we need to store the code_id? It can be retrieved from the ContractInfo (using the addr) once the contract is instantiated.

Doesn't storing it here prevent one more query in the required path though?

Doesn't storing it here prevent one more query in the required path though?

I think it does.

Also, I think we might want to store the code id for security purposes so that it is impossible to upgrade the contract to something malicious.

We can allow this to be upgradeable via gov prop. WDYT?

Going to make an issue to track this discussion

Tracking discussion here: #4779

Doesn't storing it here prevent one more query in the required path though?

Only when the code id is being used, which is only on instantiation (AFAICT). Not sure when else we would need the code id.

Also, I think we might want to store the code id for security purposes so that it is impossible to upgrade the contract to something malicious.

We can make the contract non-upgradable by giving it no admin.

Also if we wanted to enforce this by checking the code id, we would need to query the ContractInfo on every call to see that it matches what is stored.

proto/osmosis/cosmwasmpool/v1beta1/model/pool.proto

nicolaslara · 2023-03-28T12:53:51Z

proto/osmosis/poolmanager/v1beta1/query.proto

-    option (google.api.http).get =
-        "/osmosis/poolmanager/v1beta1/{pool_id}/estimate/single_pool_swap_exact_amount_in";
+    option (google.api.http).get = "/osmosis/poolmanager/v1beta1/{pool_id}/"
+                                   "estimate/single_pool_swap_exact_amount_in";


This should be one line. The linter splits it, but apparently protobufs don't support that. ref #4549 (comment)

Is this confirmed? Would be really odd if the linter did something that wasn't proto compatible...

I'm also not convinced this is a problem. I searched the repo and there are multiple places where this is happening.

If this is indeed an issue, we should investigate fixing the linter IMO as it is almost guaranteed it will get missed in one of the PRs

Going to follow-up on Slack

yeah, not sure. Just going off what Matt said. @mattverse have you confirmed that stuff breaks here? what breaks?

From what I have manually tested, I have came to a conclusion that these multiple liners are not correctly supported when they are getting converted to rest endpoints.

On the second look, it might have been just me having effect by other testing variants. Worthwhile re-testing, cannot assure that its 100% not supported

proto/osmosis/poolmanager/v1beta1/query.proto

x/cosmwasmpool/model/msgs.go

nicolaslara

Left some notes, but in general this looks good to me

czarcas7ic

LGTM 👍

czarcas7ic · 2023-03-28T18:22:48Z

proto/osmosis/cosmwasmpool/v1beta1/model/pool.proto

+  string contract_address = 2
+      [ (gogoproto.moretags) = "yaml:\"contract_address\"" ];
+  uint64 pool_id = 3;
+  uint64 code_id = 4;


Doesn't storing it here prevent one more query in the required path though?

czarcas7ic · 2023-03-28T18:27:01Z

proto/osmosis/poolmanager/v1beta1/query.proto

-    option (google.api.http).get =
-        "/osmosis/poolmanager/v1beta1/{pool_id}/estimate/single_pool_swap_exact_amount_in";
+    option (google.api.http).get = "/osmosis/poolmanager/v1beta1/{pool_id}/"
+                                   "estimate/single_pool_swap_exact_amount_in";


Is this confirmed? Would be really odd if the linter did something that wasn't proto compatible...

x/cosmwasmpool/model/msgs.go

x/cosmwasmpool/types/params.go

Co-authored-by: Adam Tucker <[email protected]>

feat: x/cosmwasmpool proto and query boilerplate

b299aa0

github-actions bot added C:CLI C:x/poolmanager labels Mar 28, 2023

p0mvn added 2 commits March 28, 2023 02:03

add basic wiring to keeper

46c2206

updates

b2fecd3

p0mvn commented Mar 28, 2023

View reviewed changes

p0mvn added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Mar 28, 2023

p0mvn commented Mar 28, 2023

View reviewed changes

x/cosmwasmpool/types/expected_keepers.go Outdated Show resolved Hide resolved

Update x/cosmwasmpool/types/expected_keepers.go

dceb80d

p0mvn mentioned this pull request Mar 28, 2023

CosmWasm Pool: Misc Tracking issue #4765

Closed

6 tasks

p0mvn requested review from nicolaslara, czarcas7ic and iboss-ptk March 28, 2023 02:35

p0mvn marked this pull request as ready for review March 28, 2023 02:38

nicolaslara reviewed Mar 28, 2023

View reviewed changes

proto/osmosis/cosmwasmpool/v1beta1/model/pool.proto Outdated Show resolved Hide resolved

nicolaslara reviewed Mar 28, 2023

View reviewed changes

proto/osmosis/poolmanager/v1beta1/query.proto Show resolved Hide resolved

nicolaslara reviewed Mar 28, 2023

View reviewed changes

x/cosmwasmpool/model/msgs.go Show resolved Hide resolved

nicolaslara approved these changes Mar 28, 2023

View reviewed changes

czarcas7ic approved these changes Mar 28, 2023

View reviewed changes

This was referenced Mar 29, 2023

CWPool: handle code id #4779

Closed

bug: fix mutli-line proto-linter problem #4780

Closed

p0mvn and others added 3 commits March 29, 2023 00:44

remove instantiate message from pool model

2e569db

Update x/cosmwasmpool/types/params.go

ce80081

Co-authored-by: Adam Tucker <[email protected]>

Update x/cosmwasmpool/types/params.go

b55dd98

Co-authored-by: Adam Tucker <[email protected]>

p0mvn and others added 2 commits March 28, 2023 20:46

Update x/cosmwasmpool/model/msgs.go

49259ce

Co-authored-by: Adam Tucker <[email protected]>

Update x/cosmwasmpool/model/msgs.go

4d07910

Co-authored-by: Adam Tucker <[email protected]>

p0mvn merged commit 1fb9da8 into main Mar 29, 2023

p0mvn deleted the roman/cosmwasm-pool-protos branch March 29, 2023 01:07

p0mvn restored the roman/cosmwasm-pool-protos branch March 29, 2023 01:12

p0mvn mentioned this pull request May 28, 2023

[CWPool]: Misc Tracking Issue #5329

Open

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: x/cosmwasmpool proto and query boilerplate #4764

feat: x/cosmwasmpool proto and query boilerplate #4764

p0mvn commented Mar 28, 2023 •

edited

Loading

p0mvn Mar 28, 2023 •

edited

Loading

p0mvn Mar 28, 2023 •

edited

Loading

p0mvn Mar 28, 2023 •

edited

Loading

nicolaslara Mar 28, 2023

nicolaslara Mar 28, 2023

p0mvn Mar 29, 2023

p0mvn Mar 28, 2023

p0mvn Mar 28, 2023 •

edited

Loading

nicolaslara Mar 28, 2023

czarcas7ic Mar 28, 2023

p0mvn Mar 29, 2023

p0mvn Mar 29, 2023

nicolaslara Mar 29, 2023

nicolaslara Mar 28, 2023

czarcas7ic Mar 28, 2023

p0mvn Mar 29, 2023 •

edited

Loading

p0mvn Mar 29, 2023

nicolaslara Mar 29, 2023

mattverse Mar 29, 2023

nicolaslara left a comment

czarcas7ic left a comment

czarcas7ic Mar 28, 2023

czarcas7ic Mar 28, 2023


		option go_package = "github.com/osmosis-labs/osmosis/v15/x/cosmwasmpool/types";

		service Msg {}

		// TODO: uncomment once merging state-breaks.
		// return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&msg))

feat: x/cosmwasmpool proto and query boilerplate #4764

feat: x/cosmwasmpool proto and query boilerplate #4764

Conversation

p0mvn commented Mar 28, 2023 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

p0mvn Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

p0mvn Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

p0mvn Mar 28, 2023 • edited Loading

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

p0mvn Mar 28, 2023 • edited Loading

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

p0mvn Mar 29, 2023 • edited Loading

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

nicolaslara left a comment

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Mar 28, 2023 •

edited

Loading

p0mvn Mar 28, 2023 •

edited

Loading

p0mvn Mar 28, 2023 •

edited

Loading

p0mvn Mar 28, 2023 •

edited

Loading

p0mvn Mar 28, 2023 •

edited

Loading

p0mvn Mar 29, 2023 •

edited

Loading