-
Notifications
You must be signed in to change notification settings - Fork 602
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
Cosmwasmpool spec #4810
Cosmwasmpool spec #4810
Conversation
7d1f8df
to
3014f77
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.
Great work and thank you @iboss-ptk .
Left some questions and comments. Let me know what you think
The CosmWasm contract that is to be instanitiated needs to implement [CosmWasm Pool Contract Interface](#cosmwasm-pool-contract-interface) and store it on chain first. Then new pool can be created by sending `MsgCreateCosmWasmPool`. | ||
|
||
|
||
`MsgCreateCosmWasmPool` contains `InstantiateMsg`, which is a message that will be passed to the CosmWasm contract when it is instantiated. The structure of the message is defined by the contract developer, and can contain any information that the contract needs to be instantiated. JSON format is used for `InstantiateMsg`. |
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.
Should we decide on how code id should be handled?
I think we should be saving the code id on chain but having the flexibility to upgrade it via gov prop. WDYT?
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.
Not sure if we need to save code id if we already save address, since we can query it from wasm module unless we have explicit need to store.
For gov prop, I think it would be nice if we can have prop type where it does
- store code and init pool
- store code and migrate
Else we need 2 separated gov prop every time adding / upgrading new pool type. It might not be very frequent but if it's not too hard to do I think it make sense to have them.
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 was thinking that it might be useful from a security standpoint.
However, on more thought, I agree that it is not needed.
We can make only admin (module account) be allowed to migrate a contract. The migration is controlled by governance.
store code and migrate
I agree on having this.
store code and init pool
Why would we also init pool? Shouldn't that be done by MsgCreateCosmWasmPool
separately?
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.
Once we agree on this, could you please update the spec for migration logic and gov props?
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.
Why would we also init pool? Shouldn't that be done by MsgCreateCosmWasmPool separately?
MsgCreateCosmWasmPool
only create pool out of existing code stored on chain.
x/cosmwasmpool/README.md
Outdated
/// GetTotalShares returns the total number of LP shares in the pool | ||
#[returns(TotalSharesResponse)] | ||
GetTotalShares {}, |
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 not on the PoolI
anymore. Ref: #4801
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.
So total pool liquidity is still in PoolModuleI
, total shares is removed from all generic interface.
What's the difference between PoolI
and PoolModuleI
?
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.
PoolI
is a pool interface that is implemented by pool models. These pool models are also written to state.
Examples of pool models:
- balancer
- stableswap
- concentrated liquidity
we are going to add a cosmwasm pool model.
PoolModuleI
is an interface implemented by the SDK module keepers encapsulating these pool models.
We have:
- x/gamm keeper
- x/concentrated-liquidity keeper
we are going to add:
- x/cosmwasm keeper
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 looks great! I think using sylvia's interfaces on transmuter will be helpful. There can be a Pool trait that any struct can implement to become a pool
Co-authored-by: Roman <[email protected]>
Exactly! Planning on extracting what described in spec as sylvia pool interface! @nicolaslara |
What is the plan with incentives for |
@p0mvn will look into it! |
Just thought of another consideration - we should probably mention how twap is handled. It probably does not make sense to have it integrated it for transmuter but might be worthwhile for other pools |
@p0mvn Add more info about incentives, I'm not really familiar with the module, let me know if I miss anything or it's unclear. |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
} | ||
``` | ||
|
||
(TBD) On how to handle the deactivation operationally. |
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 have a gov prop + sudo message for toggling the is_active flag. WDYT?
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.
totally
I propose that we do not integrate this pool type into x/twap for now. No specific code changes should be required for this but can you please update the spec? Should be as simple as mentioning that x/twap is not implemented for cosmwasm pools but can be in the future if there is a need |
TODO for me: check if we can use tokenfactory for representing shares |
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.
LGTM. Thank you @iboss-ptk
I will be submitting further updates in future PRs
* spec: creating cosmwasm pool * spec: swap * spec: deactivating * spec: cosmwasm pool contract interface * Update x/cosmwasmpool/README.md Co-authored-by: Roman <[email protected]> * decide on keeping swap on sudo and rephrase sudo's problem as spec * remove get total shares * update create pool flow per comment * update explanation on ensuring token in * update incentives & shares section * update swap_fee explanation * mention about no twap integration --------- Co-authored-by: Roman <[email protected]>
What is the purpose of the change
Adding specification to CosmWasm pool