Return IBC packet sequence number #1225

pinosu · 2023-03-02T14:38:36Z

Resolves #1154

codecov · 2023-03-02T14:52:37Z

Codecov Report

Merging #1225 (c6d5828) into main (257552f) will decrease coverage by 0.05%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #1225      +/-   ##
==========================================
- Coverage   57.49%   57.44%   -0.05%     
==========================================
  Files          54       54              
  Lines        7437     7443       +6     
==========================================
  Hits         4276     4276              
- Misses       2862     2865       +3     
- Partials      299      302       +3

Impacted Files	Coverage Δ
x/wasm/keeper/handler_plugin.go	`82.56% <50.00%> (-2.87%)`	⬇️
x/wasm/keeper/keeper.go	`87.50% <0.00%> (-0.32%)`	⬇️

alpe

Great work. Just some minor nits.
Please 👁️ at them before merging.

alpe · 2023-03-03T08:57:11Z

proto/cosmwasm/wasm/v1/ibc.proto

+
+// MsgIBCSendResponse
+message MsgIBCSendResponse {
+  // sequence number of the transfer packet sent


Suggested change

// sequence number of the transfer packet sent

// Sequence number of the IBC packet sent

alpe · 2023-03-03T10:27:20Z

proto/cosmwasm/wasm/v1/ibc.proto

+// MsgIBCSendResponse
+message MsgIBCSendResponse {
+  // sequence number of the transfer packet sent
+  uint64 sequence = 1;


alpe · 2023-03-03T10:32:07Z

x/wasm/keeper/handler_plugin.go

+	}
+
+	resp := &types.MsgIBCSendResponse{Sequence: sequence}
+	val, err := resp.Marshal()


This does work correct!
I had expected to see the codec.Codec passed to the IBCRawPacketHandler object as an abstraction layer but this is fine.

alpe · 2023-03-03T10:33:26Z

x/wasm/keeper/handler_plugin.go

+		return nil, nil, sdkerrors.Wrap(err, "failed to marshal sequence response")
+	}
+
+	data = append(data, val)


No need to append. Just set the value here

Suggested change

data = append(data, val)

data = val

or better

return nil, val, nil

alpe · 2023-03-03T10:37:08Z

x/wasm/keeper/handler_plugin_test.go

+			require.NotNil(t, data)
+
+			msg := &types.MsgIBCSendResponse{Sequence: 1}
+			val, err := msg.Marshal()


This is the same code as used in the handler. When you unmarshal the returned data instead and compare the seq, you are acting as the client. This gives confidence that both sides work.

ethanfrey

Looks good.
I'd love to see a full-stack test with contracts, but not sure if that is possible.

(We do have some ibc contract tests I believe, maybe some check there in a follow up PR)

ethanfrey · 2023-03-04T17:54:21Z

x/wasm/keeper/handler_plugin_test.go

+
+			expMsg := types.MsgIBCSendResponse{Sequence: 1}
+
+			actualMsg := types.MsgIBCSendResponse{}


Nice test case

ethanfrey · 2023-03-04T17:55:08Z

x/wasm/keeper/handler_plugin_test.go

 			// then
 			require.True(t, spec.expErr.Is(gotErr), "exp %v but got %#+v", spec.expErr, gotErr)
 			if spec.expErr != nil {
 				return
 			}
-			assert.Nil(t, data)
+
 			assert.Nil(t, evts)


Really? The events are nil? How do we get the ibc packet events to the relayers here?
(I guess they just pass through context and not via the return value?)

yes, via context.

webmaster128

Very nice, two small nits

webmaster128 · 2023-03-06T08:32:18Z

proto/cosmwasm/wasm/v1/ibc.proto

+message MsgIBCSendResponse {
+  // Sequence number of the IBC packet sent
+  uint64 sequence = 1;
+}


proto/cosmwasm/wasm/v1/tx.proto is not super consistent but usually we have the response type right after the message type. I think that's helpful for navigating. I'd move it above message MsgIBCCloseChannel {.

webmaster128 · 2023-03-06T08:35:20Z

x/wasm/keeper/handler_plugin.go

+	resp := &types.MsgIBCSendResponse{Sequence: sequence}
+	val, err := resp.Marshal()
+	if err != nil {
+		return nil, nil, sdkerrors.Wrap(err, "failed to marshal sequence response")


Suggested change

return nil, nil, sdkerrors.Wrap(err, "failed to marshal sequence response")

return nil, nil, sdkerrors.Wrap(err, "failed to marshal IBC send response")

sonarqubecloud · 2023-03-06T08:48:43Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

No Coverage information
15.8% Duplication

* Return IBC packet sequence number * Fix review feedbacks * Remove names to return values in DispatchMsg method * Fix comments (cherry picked from commit 4f1c57f) # Conflicts: # x/wasm/keeper/handler_plugin.go

* Return IBC packet sequence number (#1225) * Return IBC packet sequence number * Fix review feedbacks * Remove names to return values in DispatchMsg method * Fix comments (cherry picked from commit 4f1c57f) # Conflicts: # x/wasm/keeper/handler_plugin.go * Fix merge conflict --------- Co-authored-by: pinosu <[email protected]> Co-authored-by: Alex Peters <[email protected]>

* main: (36 commits) Add changelog for v0.31.0 (#1188) Upgrade to wasmvm 1.2.1 (#1245) Bump bufbuild/buf-setup-action from 1.15.0 to 1.15.1 Make `CaptureIbcEvents` in ibctesting public. Fix client checksum verification (#1234) Test rust panic for regression Return IBC packet sequence number (#1225) Rename windows client binary Configure sonarcloud analysis Bump github.com/cosmos/gogoproto from 1.4.3 to 1.4.6 Add Windows client support (#1197) Bump github.com/cosmos/cosmos-proto from 1.0.0-beta.1 to 1.0.0-beta.2 Bump bufbuild/buf-setup-action from 1.14.0 to 1.15.0 Bump SDK version to v0.45.14 Bump bufbuild/buf-setup-action from 1.13.1 to 1.14.0 (#1200) Bump github.com/stretchr/testify from 1.8.1 to 1.8.2 (#1211) test: add unit test fix: stargate querier does not reset the state list-contract-by-code bugfix update interface proto annotations (#1156) ...

* Start cosmos-sdk v0.47 integration (#1136) * Upgrade to sdk v0.47 branch * More integration work * SDK version upgrade; fixes * More fixes * Fixes * Deactivate failing tests * SDK + ibc-go version upgrades * limix gas fix (cherry picked from commit f7f8417) * with valset in bench (cherry picked from commit 35b2a8f) * Revert staking query handler; fix tests * Minor cleanup * Rebased * Address linter issues * Set legacy router proper * Deactivate failing test. Race condition needs to handled in SDK * Address some code smells * Bump sdk version * Use gov v1 internally for votes * Activate test after sdk fix * Add group test * Add config template for wasm fields * Add Rust backtrace flag for more debug output on simulations * Set unique node folder for tests * Revert "Add Rust backtrace flag for more debug output on simulations" This reverts commit 218c3c6. * Simulations * Run also im/export + deterministic sims * Add package prefix to interfaces * Add signer annotation (cosmos/cosmos-sdk#10933), minor cleanup * Bump sdk version * Review comments Co-authored-by: vuong <[email protected]> * Bump bufbuild/buf-setup-action from 1.11.0 to 1.12.0 Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.11.0 to 1.12.0. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.11.0...v1.12.0) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> (cherry picked from commit f490595) * Remove intertx for vanilla ICA * fix msg format in EVENTS.md (cherry picked from commit 38d466a) * Better to sdk coin convertion (#1164) * Better to sdk coin convertion * Review feedback (cherry picked from commit a925a9e) * Disallow only address permission (#1163) * Remove AccessTypeOnlyAddress for store msg * Remove AccessTypeOnlyAddress for update config msg * Review feedback Co-authored-by: Simon Warta <[email protected]> Co-authored-by: Simon Warta <[email protected]> (cherry picked from commit 8991633) * Integrate wasmvm v1.2.0 (backport #1161) (#1175) * Integrate wasmvm v1.2.0 (#1161) * Bump wasmvm version * Bump wasm test contracts * Encode weighted votes * Encode instantiate2 * Handle code info query; better wasmvm errors * Fix readme * Make linter happy * add non cgo build * Review comments * Bump wasmvm to release version Co-authored-by: jhernandezb <[email protected]> (cherry picked from commit 957b38e) # Conflicts: # x/wasm/keeper/handler_plugin_encoders.go # x/wasm/keeper/handler_plugin_encoders_test.go # x/wasm/keeper/keeper.go # x/wasm/keeper/keeper_test.go * Adress merge conflicts Co-authored-by: Alexander Peters <[email protected]> * Bump bufbuild/buf-setup-action from 1.12.0 to 1.13.0 Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.12.0 to 1.13.0. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.12.0...v1.13.0) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> (cherry picked from commit ffa0e5e) * Emit events for setContractAdmin + setAccessConfig (#1179) (cherry picked from commit c9e7830) * Dependency upgrades (#1172) * Bump sdk version to lastest * Bump ibc-go version to lastest * Remove channel hack * Update to ibc-go v7 + protoVer=0.11.5 * Bump bufbuild/buf-setup-action from 1.13.0 to 1.13.1 Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.13.0 to 1.13.1. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.13.0...v1.13.1) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> (cherry picked from commit de27e7f) * Fix typos (backport #1185) (#1194) * Fix typos (cherry picked from commit c88b819) # Conflicts: # proto/cosmwasm/wasm/v1/tx.proto * Fix merge conflict --------- Co-authored-by: Alex Peters <[email protected]> * Bump bufbuild/buf-setup-action from 1.13.1 to 1.14.0 (#1200) Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.13.1 to 1.14.0. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.13.1...v1.14.0) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit f3fc31c) * list-contract-by-code bugfix (cherry picked from commit 2ccffed) * fix: stargate querier does not reset the state (cherry picked from commit fd03235) * test: add unit test (cherry picked from commit 6d8018a) * Add Windows client support (#1197) * Add Windows client support * Separate server and windows client --------- Co-authored-by: Alex Peters <[email protected]> (cherry picked from commit 8a20779) * Bump bufbuild/buf-setup-action from 1.14.0 to 1.15.0 Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.14.0 to 1.15.0. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.14.0...v1.15.0) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> (cherry picked from commit e5fab3d) * Rename windows client binary (cherry picked from commit de09c7f) * Return IBC packet sequence number (backport #1225) (#1233) * Return IBC packet sequence number (#1225) * Return IBC packet sequence number * Fix review feedbacks * Remove names to return values in DispatchMsg method * Fix comments (cherry picked from commit 4f1c57f) # Conflicts: # x/wasm/keeper/handler_plugin.go * Fix merge conflict --------- Co-authored-by: pinosu <[email protected]> Co-authored-by: Alex Peters <[email protected]> * Test rust panic for regression (cherry picked from commit a52e604) * Fix client checksum verification (#1234) * Fix client checksum verification * Review comments (cherry picked from commit 1a8019b) # Conflicts: # x/wasm/client/cli/gov_tx.go * Fix merge conflict * Fix linters * Configure sonarcloud analysis (cherry picked from commit 85cf161) * Bump bufbuild/buf-setup-action from 1.15.0 to 1.15.1 Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.15.0 to 1.15.1. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.15.0...v1.15.1) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> (cherry picked from commit 730ea5a) * Make `CaptureIbcEvents` in ibctesting public. Before this change, it wasn't possible to implement the `chain.SendMsgs` method without [copying](https://github.com/public-awesome/ics721/blob/main/e2e/suite_helpers.go#L81-L98) them over. (cherry picked from commit b64fa07) * Upgrade to wasmvm 1.2.1 (backport #1245) (#1254) * Upgrade to wasmvm 1.2.1 (#1245) * Use wasmvm store adapter * Bump wasmvm to v1.2.1 (cherry picked from commit 850f901) # Conflicts: # go.mod # go.sum # x/wasm/keeper/keeper.go * Resolve conflicts --------- Co-authored-by: Alexander Peters <[email protected]> * WIP All cometbft (#1244) * Dep upgrade; use CometBft * Remove duplicte message events * Add changelog for v0.31.0 (#1188) * Start changelog for v0.31.0 * Add ICA upgrade * Add proto version link to buf.build * Update changelog (#1239) * Update changelog * Update changelog with latest changes * Set release date --------- Co-authored-by: pinosu <[email protected]> (cherry picked from commit bc0e817) * Remove new message type event * Support msg update params gov proposal (#1247) * Add MsgUpdateParams support * Implement UpdateParams msg * Fix test UpdateParams * Add migration test * Fix * Fix lint issues * Revert changes according to review feedback * Remove more x/params dependencies * Remove x/params from genesis test * Formatting * Restore old changes * fix lint * Fix tests and restructure migrations * Rename alias for convention --------- Co-authored-by: Alex Peters <[email protected]> * Fix test data generator (#1263) * linting 47 pr (#1261) * lint cosmwasm for sdk 47 * fix * remove setGenesis * remove additional unused functions * pass tests * use SDK's errors module * unecessary conversions * unnecessary conversions * remove unneeded event manager * complete linting of tests for 47 * add test for reimportation * check errors * Update x/wasm/keeper/proposal_integration_test.go Co-authored-by: Alexander Peters <[email protected]> * apply suggestion * suggestions * lints * don't return error in when making new transactions * no todo's in the code * Fix test data generator * Update x/wasm/types/genesis_test.go Co-authored-by: Alexander Peters <[email protected]> * use the full string invalid address (2 words) always --------- Co-authored-by: Alexander Peters <[email protected]> * Regenerate from proto; remove dead code; polish code * Set SDK version to v0.47x.0 (#1262) * Set SDK version to v0.47x.0 * Set chainID * Minor updates * Set chainID for simulations * Buf mod update * Use sdk tag instead of hash in buf * Bump ibc-go to v7.0.0 * faddat/re merge main (#1274) undefined --------- Co-authored-by: vuong <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: llllllluc <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Gjermund Garaba <[email protected]> Co-authored-by: Nikhil Suri <[email protected]> Co-authored-by: Paul <[email protected]> Co-authored-by: pinosu <[email protected]> Co-authored-by: ekez <[email protected]> Co-authored-by: Jacob Gadikian <[email protected]>

Return IBC packet sequence number

fb40d0b

pinosu requested a review from alpe as a code owner March 2, 2023 14:38

alpe approved these changes Mar 3, 2023

View reviewed changes

alpe added the backport/sdk47-dev label Mar 3, 2023

pinosu added 2 commits March 3, 2023 15:10

Fix review feedbacks

9ad2807

Remove names to return values in DispatchMsg method

819d945

alpe requested review from webmaster128 and ethanfrey March 4, 2023 08:31

ethanfrey approved these changes Mar 4, 2023

View reviewed changes

webmaster128 reviewed Mar 6, 2023

View reviewed changes

Fix comments

c6d5828

pinosu merged commit 4f1c57f into main Mar 6, 2023

mergify bot mentioned this pull request Mar 6, 2023

Return IBC packet sequence number (backport #1225) #1233

Merged

alpe deleted the 1154-return_sequence_number branch May 10, 2023 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return IBC packet sequence number #1225

Return IBC packet sequence number #1225

pinosu commented Mar 2, 2023

codecov bot commented Mar 2, 2023 •

edited

Loading

alpe left a comment

alpe Mar 3, 2023

alpe Mar 3, 2023

alpe Mar 3, 2023

alpe Mar 3, 2023

alpe Mar 3, 2023

ethanfrey left a comment

ethanfrey Mar 4, 2023

ethanfrey Mar 4, 2023

alpe Mar 6, 2023

webmaster128 left a comment

webmaster128 Mar 6, 2023

webmaster128 Mar 6, 2023

sonarqubecloud bot commented Mar 6, 2023

	// sequence number of the transfer packet sent
	// Sequence number of the IBC packet sent


		expMsg := types.MsgIBCSendResponse{Sequence: 1}

		actualMsg := types.MsgIBCSendResponse{}

	return nil, nil, sdkerrors.Wrap(err, "failed to marshal sequence response")
	return nil, nil, sdkerrors.Wrap(err, "failed to marshal IBC send response")

Return IBC packet sequence number #1225

Return IBC packet sequence number #1225

Conversation

pinosu commented Mar 2, 2023

codecov bot commented Mar 2, 2023 • edited Loading

Codecov Report

alpe left a 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

ethanfrey left a 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

webmaster128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 6, 2023

codecov bot commented Mar 2, 2023 •

edited

Loading