Integrate Packet Forward Middleware #3911
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,17 @@ package e2e | |
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "github.com/iancoleman/orderedmap" | ||
| "github.com/osmosis-labs/osmosis/v14/tests/e2e/configurer/chain" | ||
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
|
|
||
| packetforwardingtypes "github.com/strangelove-ventures/packet-forward-middleware/v4/router/types" | ||
|
|
||
| ibchookskeeper "github.com/osmosis-labs/osmosis/x/ibc-hooks/keeper" | ||
|
|
||
| paramsutils "github.com/cosmos/cosmos-sdk/x/params/client/utils" | ||
|
|
@@ -25,6 +29,30 @@ import ( | |
| "github.com/osmosis-labs/osmosis/v14/tests/e2e/initialization" | ||
| ) | ||
|
|
||
| // Reusable Checks | ||
|
|
||
| // CheckBalance Checks the balance of an address | ||
| func (s *IntegrationTestSuite) CheckBalance(node *chain.NodeConfig, addr string, amount int64) { | ||
| // check the balance of the contract | ||
| s.Eventually(func() bool { | ||
nicolaslara marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| balance, err := node.QueryBalances(addr) | ||
| s.Require().NoError(err) | ||
| if len(balance) == 0 { | ||
| return false | ||
| } | ||
| // check that the amount is in one of the balances inside the balance list | ||
| for _, b := range balance { | ||
| if b.Amount.Int64() == amount { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| }, | ||
| 1*time.Minute, | ||
| 10*time.Millisecond, | ||
|
There was a problem hiding this comment. I think that CI might be flaky and blocking requests due to retrying this too frequently. I've run into similar issues before where AFAIR increasing this number helped. I looked into the node logs: https://github.com/osmosis-labs/osmosis/suites/10828110105/artifacts/545247532 and they seem okay. So I'm guessing it might be some setup problem There was a problem hiding this comment. Tried increasing it and it didn't help, but somehow it works (locally) after I added the |
||
| ) | ||
| } | ||
|
|
||
| func (s *IntegrationTestSuite) TestConcentratedLiquidity() { | ||
| chainA := s.configurer.GetChainConfig(0) | ||
| node1, err := chainA.GetDefaultNode() | ||
|
|
@@ -292,55 +320,52 @@ func (s *IntegrationTestSuite) TestIBCTokenTransferRateLimiting() { | |
| node.WasmExecute(contracts[0], `{"remove_path": {"channel_id": "channel-0", "denom": "uosmo"}}`, initialization.ValidatorWalletName) | ||
| } | ||
|
|
||
| func (s *IntegrationTestSuite) UploadAndInstantiateCounter(chain *chain.Config) string { | ||
| // copy the contract from tests/ibc-hooks/bytecode | ||
| wd, err := os.Getwd() | ||
| s.NoError(err) | ||
| // co up two levels | ||
| projectDir := filepath.Dir(filepath.Dir(wd)) | ||
| err = copyFile(projectDir+"/tests/ibc-hooks/bytecode/counter.wasm", wd+"/scripts/counter.wasm") | ||
| s.NoError(err) | ||
| node, err := chain.GetDefaultNode() | ||
| s.NoError(err) | ||
|
|
||
| node.StoreWasmCode("counter.wasm", initialization.ValidatorWalletName) | ||
| chain.LatestCodeId = int(node.QueryLatestWasmCodeID()) | ||
| node.InstantiateWasmContract( | ||
| strconv.Itoa(chain.LatestCodeId), | ||
| `{"count": 0}`, | ||
| initialization.ValidatorWalletName) | ||
|
|
||
| contracts, err := node.QueryContractsFromId(chain.LatestCodeId) | ||
| s.NoError(err) | ||
| s.Require().Len(contracts, 1, "Wrong number of contracts for the counter") | ||
| contractAddr := contracts[0] | ||
| return contractAddr | ||
| } | ||
|
|
||
| func (s *IntegrationTestSuite) TestIBCWasmHooks() { | ||
| if s.skipIBC { | ||
| s.T().Skip("Skipping IBC tests") | ||
| } | ||
| chainA := s.configurer.GetChainConfig(0) | ||
| chainB := s.configurer.GetChainConfig(1) | ||
|
|
||
| nodeA, err := chainA.GetDefaultNode() | ||
| s.NoError(err) | ||
| nodeB, err := chainB.GetDefaultNode() | ||
| s.NoError(err) | ||
|
|
||
| contractAddr := s.UploadAndInstantiateCounter(chainA) | ||
|
|
||
| transferAmount := int64(10) | ||
| validatorAddr := nodeB.GetWallet(initialization.ValidatorWalletName) | ||
| nodeB.SendIBCTransfer(validatorAddr, contractAddr, fmt.Sprintf("%duosmo", transferAmount), | ||
| fmt.Sprintf(`{"wasm":{"contract":"%s","msg": {"increment": {}} }}`, contractAddr)) | ||
|
|
||
| // check the balance of the contract | ||
| s.CheckBalance(nodeA, contractAddr, transferAmount) | ||
|
|
||
| // sender wasm addr | ||
| senderBech32, err := ibchookskeeper.DeriveIntermediateSender("channel-0", validatorAddr, "osmo") | ||
|
|
@@ -359,6 +384,57 @@ func (s *IntegrationTestSuite) TestIBCWasmHooks() { | |
| ) | ||
| } | ||
|
|
||
| // TestPacketForwarding sends a packet from chainA to chainB, and forwards it | ||
| // back to chainA with a custom memo to execute the counter contract on chain A | ||
| func (s *IntegrationTestSuite) TestPacketForwarding() { | ||
ValarDragon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if s.skipIBC { | ||
| s.T().Skip("Skipping IBC tests") | ||
| } | ||
| chainA := s.configurer.GetChainConfig(0) | ||
|
|
||
| nodeA, err := chainA.GetDefaultNode() | ||
| s.NoError(err) | ||
|
|
||
| // Instantiate the counter contract on chain A | ||
| contractAddr := s.UploadAndInstantiateCounter(chainA) | ||
|
|
||
| transferAmount := int64(10) | ||
| validatorAddr := nodeA.GetWallet(initialization.ValidatorWalletName) | ||
| // Specify that the counter contract should be called on chain A when the packet is received | ||
| contractCallMemo := []byte(fmt.Sprintf(`{"wasm":{"contract":"%s","msg": {"increment": {}} }}`, contractAddr)) | ||
| // Generate the forward metadata | ||
| forwardMetadata := packetforwardingtypes.ForwardMetadata{ | ||
| Receiver: contractAddr, | ||
| Port: "transfer", | ||
| Channel: "channel-0", | ||
| Next: packetforwardingtypes.NewJSONObject(false, contractCallMemo, orderedmap.OrderedMap{}), // The packet sent to chainA will have this memo | ||
| } | ||
| memoData := packetforwardingtypes.PacketMetadata{Forward: &forwardMetadata} | ||
| forwardMemo, err := json.Marshal(memoData) | ||
| s.NoError(err) | ||
| // Send the transfer from chainA to chainB. ChainB will parse the memo and forward the packet back to chainA | ||
| nodeA.SendIBCTransfer(validatorAddr, validatorAddr, fmt.Sprintf("%duosmo", transferAmount), string(forwardMemo)) | ||
|
|
||
| // check the balance of the contract | ||
| s.CheckBalance(nodeA, contractAddr, transferAmount) | ||
|
|
||
| // sender wasm addr | ||
| senderBech32, err := ibchookskeeper.DeriveIntermediateSender("channel-0", validatorAddr, "osmo") | ||
| var response map[string]interface{} | ||
| s.Eventually(func() bool { | ||
| response, err = nodeA.QueryWasmSmart(contractAddr, fmt.Sprintf(`{"get_count": {"addr": "%s"}}`, senderBech32)) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| count := response["count"].(float64) | ||
| // check if denom contains "uosmo" | ||
| return err == nil && count == 0 | ||
|
There was a problem hiding this comment. We need to handle this TODO right? IMO we should ensure exact osmo amount received, in order to be certain that our default post-upgrade parameter has no fee taken. There was a problem hiding this comment. I think that comment is a leftover from previous code. The balance check is done above (line419) but it's not checking the denom, just the amount. I'll add the denom check. |
||
| }, | ||
| 15*time.Second, | ||
| 10*time.Millisecond, | ||
| ) | ||
| } | ||
|
|
||
| // TestAddToExistingLockPostUpgrade ensures addToExistingLock works for locks created preupgrade. | ||
| func (s *IntegrationTestSuite) TestAddToExistingLockPostUpgrade() { | ||
| if s.skipUpgrade { | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Uhh, why does it need this? Community pool?
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.
yeah, the fee can be configured via a param as a percentage of the send and it funds the community pool.
Ideally this should go to stakers IMO, but the fee defaults to 0, so we can change that later
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.
RIP, this is bad module design imo, should be a middleware responsibility. (Fee at the base transport layer bad design)
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.
Can you please expand on this @ValarDragon ? Would love to understand your perspective so we can improve it. I'm not sure what you mean, as this is the keeper for the IBC middleware.
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 don't see why your transport layer for packet sending should have bips of fee bundled in at default. This is equivalent in design practice to saying our API for bank sends should take in a fee parameter as well, and have bips of all sends be captured.
You could in principle want this, but this can be pushed to wrappers of the main object, rather than being the main thing you import. (I'd prefer no capability of this in what we import, than having such a parameter forced)
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.
(NOTE: This is very minor on the overall work going into this. Awesome job on the module! And I think my first comment "bad module design imo" was too harsh for the severity of opinion actually held)