Custom CosmWasm Integration #1135
Conversation
* Add custom querier for osmosis wasm queries * Move tests to own dir to avoid cyclic deps * Rename wasm keeper to (custom) query plugin * Use helpers to convert from sdk to wasm coins
* Add osmo_reflect contract * Start test setup for custom queries * Fix tests * Complete initial query test * Rename bindings package to bindings * Test creating and querying 2 pools * Rename package to wasmbindings * Download wasm uses osmo-bindings version
* Add placeholder for message handler * Implement SwapMsg encoder * First table test for swapping message * Cover basic ExactIn cases * Test ExactOut swaps as well * Update error message Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>
* Add spot price query * Rename keeper.go to queries.go * Add estimate price query * Remove needless ViewKeeper interface * GetSpotPrice refactor * EstimatePrice refactor * Better EstimatePrice error message * Add TODO for multi-hop swap price estimation * Add SpotPrice happy path integration test * Remove FIXMEs * Add json field names
Add validation helpers
* Convert messenge encoder to message decorator * Implement and Test mint token message * Add TODOs for future refactoring * Update app/wasm/message_plugin.go Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com> Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>
* Fix: sender address format * Add estimate price integration tests * Remove FIXME * Improve estimate swap rate integ tests
* Add type conversions * Refactor EstimatePrice to use same logic as SwapMsg
* Add exact in multi-hop tests * Tests pass multi-hop exact in * Add failing tests for ExactOut * FImplement ExactOut multi-hop swaps * Remove two outdated TODO comments * Remove obsolete comments
* Add full denom unit tests * Add get pool state unit tests * Add get spot price unit tests * Add estimate price extra checks * Add estimate price unit tests * Add tests for zero and negative amounts * Fix: null spot price * Fix: Avoid panic on negative amounts Fix rebase errors / missing null checks * Fix rebase errors Add negative amount checks
* Add subdenom verification * Remove TODO
* Fix: swap tokens error handling * Rename messenger to CustomMessenger * Add perform swap unit tests * Fix: perform swap null swap check * Add PerformMint function * Add perform mint unit tests * Fixes: perform mint null mint check Mint negative amount
* Fix: swap tokens error handling * Rename messenger to CustomMessenger * Add perform swap unit tests * Fix: perform swap null swap check * Add PerformMint function * Add perform mint unit tests * Fixes: perform mint null mint check Mint negative amount * Update test contract to v0.4.0 * Rename EstimatePrice binding to EstimateSwap * Rename EstimatePrice custom query to EstimateSwap * Adapt test to EstimatePrice to EstimateSwap renaming
* Add multi-hop swap unit test * Adjust input amount, min output Add slippage factor * Add more multi-hop unit tests * Improve estimates in multi-hop tests * Test that self-swap is disallowed Co-authored-by: Ethan Frey <ethanfrey@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #1135 +/- ##
==========================================
- Coverage 21.08% 21.06% -0.02%
==========================================
Files 194 196 +2
Lines 25277 25301 +24
==========================================
Hits 5329 5329
- Misses 18987 19011 +24
Partials 961 961
Continue to review full report at Codecov.
|
| @@ -367,7 +369,10 @@ func (app *OsmosisApp) InitNormalKeepers( | |||
|
|
|||
| // The last arguments can contain custom message handlers, and custom query handlers, | |||
| // if we want to allow any custom callbacks | |||
| supportedFeatures := "iterator,staking,stargate" | |||
| supportedFeatures := "iterator,staking,stargate,osmosis" | |||
There was a problem hiding this comment.
We add the extra feature here, so contract including osmo-bindings are supported here.
| supportedFeatures := "iterator,staking,stargate" | ||
| supportedFeatures := "iterator,staking,stargate,osmosis" | ||
|
|
||
| wasmOpts = append(owasm.RegisterCustomPlugins(app.GAMMKeeper, app.BankKeeper), wasmOpts...) |
There was a problem hiding this comment.
And here we register the custom plugins to handle those messages
|
|
||
| var _ wasmkeeper.Messenger = (*CustomMessenger)(nil) | ||
|
|
||
| func (m *CustomMessenger) DispatchMsg(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) ([]sdk.Event, [][]byte, error) { |
There was a problem hiding this comment.
This is the entry point to handle messages.
If you add another variant to OsmosisMsg, you can add the handler here.
| PoolId: swap.First.PoolId, | ||
| TokenInDenom: swap.First.DenomIn, | ||
| }} | ||
| output := swap.First.DenomOut |
There was a problem hiding this comment.
Note that we simplify the Rust API such that it always flows from first to last (even if we set ExactOut), and then convert to the different Go API here. This shows how we can easily adapt to new APIs as long as the same basic functionality is exposed.
app/wasm/message_plugin.go
Outdated
| } | ||
|
|
||
| // GetFullDenom is a function, not method, so the message_plugin can use it | ||
| func GetFullDenom(contract string, subDenom string) (string, error) { |
There was a problem hiding this comment.
This is where we build the custom denom if the contract mints a coin.
I assume this is enough, but if you need metadata registration or other such, please let us know.
| RegenPool uint64 | ||
| } | ||
|
|
||
| func TestSwapMsg(t *testing.T) { |
There was a problem hiding this comment.
Probably the most interesting test for me.
Full stack table test where a caller requests the contract to perform a swap (while sending tokens to it), and then checking the result of the swap (balance remaining in the contract).
| expectErr: true, | ||
| }, | ||
|
|
||
| // FIXME: this panics in GAMM module !?! hits a known TODO |
There was a problem hiding this comment.
I hit this two times with ExactOut. It seems that if you trade from A -> B where 1 A is worth > 2 B then the ExactOut swaps will all fail.
When I reverse it such that the output is the more valuable currency, then ExactOut passes.
| return &wasmbindings.SwapMsg{ | ||
| First: wasmbindings.Swap{ | ||
| PoolId: state.AtomPool, | ||
| DenomIn: "uosmo", |
There was a problem hiding this comment.
Like here, OSMO -> ATOM works with ExactOut.
The above is a bug in the GAMM code (with existing TODO). I think it would be nice to fix, but that is out of scope of this PR
| }, | ||
| }, | ||
|
|
||
| "exact in: 2 step multi-hop": { |
There was a problem hiding this comment.
Showing multi-hops work properly from contracts.
| sdk.NewInt64Coin("uatom", 1999999), | ||
| }, | ||
| }, | ||
| "exact out: 2 step multi-hop": { |
There was a problem hiding this comment.
Even when setting ExactOut (where we mangle the args to call into x/gamm)
| type PoolAssets struct { | ||
| Assets []sdk.Coin | ||
| Shares sdk.Coin | ||
| } |
There was a problem hiding this comment.
As part of the refactor, we're trying to convert a lot of instances of PoolAssets in the code / other modules, to be PoolLiquidity. We should probably revisit this wasm binding pre-release
There was a problem hiding this comment.
We can gladly rename everything. Changing the type I easy and this is only used when querying the gamm, so please do suggest new names.
We use the Rust definitions as the canonical source for all types that hit the contract boundary, and this is more permanent (breaking API changes), so maybe you can double-check we use the proper names there first. Types that are only used within this package can easily be renamed by anyone
There was a problem hiding this comment.
Awesome! Will review those in more depth then
|
This is only failing linter, but not sure how to run that locally (and there is no clear error message in the CI). Any tips? |
|
So our original thinking re: native token minting was to make a "tokenfactory" module #584 / #1029 . Would it be possible / a blocker to comment out or otherwise disable the Mint tokens API for now, and re-enable that once the token factory gets on mainnet? The goal right now is to get a mainnet upgrade out around April 8th / applied on chain week of April 15th. So we likely can't get token factory in on that time frame. Should we aim to get osmo bindings in for swaps, just without minting tokens? Related: how careful do we need to be around backwards compatibility? (Are the bindings versioned) |
|
Re linting: The error here appears to be trailing newlines, so I think |
Sure.
The bindings are versioned. That's what the |
This is commented out. Since the commits will be squashed and this will not be in the git history, I left this in as comments. When the token factory is added, we can add osmosis-v2 bindings that contain this in addition (and all v1 functionality) |
| // FullDenom *FullDenom `json:"full_denom,omitempty"` | ||
| /// For a given pool ID, list all tokens traded on it with current liquidity (spot). | ||
| /// As well as the total number of LP shares and their denom. | ||
| PoolState *PoolState `json:"pool_state,omitempty"` |
There was a problem hiding this comment.
Should we rename these types to have a Query suffix?
There was a problem hiding this comment.
Is that the norm across the project? If not, I would leave it as it is.
There was a problem hiding this comment.
That is the norm for all of the golang queries, I don't know whats the norm for the cosmwasm side
There was a problem hiding this comment.
We typically label them the same name as the field.
Again, as long as the json stays the same, you can gladly rename the types as you wish in another PR.
These are for use inside the osmosis Golang code, so it should be whatever idiom looks good to you.
|
Merged main and fixed conflicts. I will merge PR once CI passes. |
Description
This follows up on the earlier CosmWasm integration work and adds custom bindings for Osmosis AMM functionality.
Contract Bindings
The other half of the work lives in https://github.com/confio/osmosis-bindings (which we are happy to transfer to the osmosis org when this is merged). If you import
osmo-bindingsin your contract, it will get an Osmosis-specific feature flag (so it will be rejected on upload by other chains), and gets access to some custom functionality. In particular, messages to Swap or Mint native tokens. As well as various queries on the AMM. We are currently missing queries for TWAP as that is not yet implemented, but we will add such bindings in a future PR when that feature is stable in the main branch.These APIs are stable and the contracts will look the same even if the keepers and Go data types change (I just had to do one such fix right before merge), as this PR serves as a translation layer from contracts to the AMM. The Rust bindings comes with a sample contract, which we upload and use in a lot of CI tests in this PR to ensure the whole flow (contract -> Osmosis -> contract) is working correctly.
Go adapters
I will comment key points in the PR, but we update OsmosisApp to register custom Wasm query and message handlers. All code is stored in
app/wasm, which exposes high level interfaces that are imported by the OsmosisApp.message_plugin.go- Code to handle theOsmosisMsgtypes returned and make native callsqueries.goandquery_plugin.go- Code to handle theOsmosisQuerytype and return proper datawasm.gothe high level binding that can be imported byOsmosisAppto register all the handlersbindings/- Go types to match the Rust types exported by https://github.com/confio/osmosis-bindings (same JSON representation)testdata/- Compiled wasm test contract and scripts to updatetest/- CI tests to ensure basic contract functionality (from initial permissioned integration) as well as all the custom callback handlers.For contributor use:
docs/) or specification (x/<module>/spec/)Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorer