Added tests of XCS with CW20s #4941
Conversation
nicolaslara
added
V:state/compatible/no_backport
|
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! |
| case CW20toA: | ||
| sender = suite.chainB | ||
| case AtoCW20: | ||
| sender = suite.chainA |
There was a problem hiding this comment.
nit: might be helpful for readability if we call this B-CW20 so the reviewer knows the CW chain is analogous to B
There was a problem hiding this comment.
I can rename it, but we can't use -. Any preference between BCW20ToA, B_CW20ToA or Bcw20ToA? They all look ugly to me
| if len(memo) == 0 { | ||
| memo = "\"\"" | ||
| } |
There was a problem hiding this comment.
for my knowledge, why is the memo required?
There was a problem hiding this comment.
because the empty string would Sprintf to nothing and then the json would say "memo": } which is invalid. We could remove the memo key altogether, but thought this was cleaner than two srpintfs
tests/ibc-hooks/xcs_cw20_test.go
Outdated
| x, ok := sdk.NewIntFromString(res.Get("balance").String()) | ||
| if !ok { | ||
| panic("could not parse balance") | ||
| } | ||
| return x |
tests/ibc-hooks/xcs_cw20_test.go
Outdated
|
|
||
| // Test that the cw20-ics20 contract can be instantiated and used to send tokens to chainA | ||
| func (suite *HooksTestSuite) TestCW20ICS20() { | ||
| suite.Require().Equal("stake", sdk.DefaultBondDenom) // hardcodiing "stake" for simplicity |
There was a problem hiding this comment.
Not sure I understand, wouldn't it just be better to utilize sdk.DefaultBondDenom instead of explicitly using stake?
There was a problem hiding this comment.
you're right. There were other places where "stake" was being used and I was too lazy to change it. Fixed now
tests/ibc-hooks/xcs_cw20_test.go
Outdated
| suite.coordinator.Setup(path) | ||
|
|
||
| osmosisAppB := chainB.GetOsmosisApp() | ||
| //osmosisAppA := chainA.GetOsmosisApp() |
There was a problem hiding this comment.
| //osmosisAppA := chainA.GetOsmosisApp() |
tests/ibc-hooks/xcs_cw20_test.go
Outdated
| osmosisAppB := chainB.GetOsmosisApp() | ||
| //osmosisAppA := chainA.GetOsmosisApp() | ||
|
|
||
| // Send some cwtoken tokens from B to A via the new path |
There was a problem hiding this comment.
| // Send some cwtoken tokens from B to A via the new path | |
| // Send some cwtoken tokens from B to A via the new path |
tests/ibc-hooks/xcs_cw20_test.go
Outdated
| chainB.SenderAccount.GetAddress(), | ||
| ) | ||
| xcsMsg := fmt.Sprintf(`{"wasm": {"contract": "%s", "msg": %s } }`, crosschainAddr, swapMsg) | ||
| serializedMemo, _ := json.Marshal(xcsMsg) |
There was a problem hiding this comment.
not sure if we are nulling err check here or if this is something else
There was a problem hiding this comment.
yeah, we're ignoring the error cause that should never fail (it's marshalling a hardcoded string (but I've changed it to checking the error to make it less confusing)
| // Check that the receiver has more cw20 tokens than before | ||
| newCw20Balance := suite.getCW20Balance(chainB, cw20Addr, receiver2) | ||
| suite.Require().Greater(newCw20Balance.Int64(), cw20Balance.Int64()) |
There was a problem hiding this comment.
Why can't we determine the exact amount expected here?
There was a problem hiding this comment.
We can do it, but since this depends on the underlying pool, any future changes to how pools work or the params we're using for the tests would make this fail. So I figure this was safer. Happy to change it if you think that's better
* added initial cw20 testing * fixed serialization * added route and fixed pool discrepancies * added cw20 tests * fixed nits
What is the purpose of the change
Test that XCS works with cw20s. This depends on CosmWasm/cw-plus#868.
On mainnet, the ics20 contract would need to be updated to use those changes
Brief Changelog
Added tests using cw20s
Testing and Verifying
tests pass
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? (yes / no)x/<module>/spec/) / Osmosis docs repo / not documented)