Consolidate usage of NewErrorAcknowledgement #1565

chatton · 2022-06-22T09:34:31Z

Description

This pr:

removes transfertypes.NewErrorAcknowledgement in favour of channeltypes.NewErrorAcknowledgement
removes interchainaccounts.NewErrorAcknowledgement in favour of channeltypes.NewErrorAcknowledgement
refactors channeltypes.NewErrorAcknowledgement to accept an error instead of a string to prevent breaking changes.

closes: #819

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

…ounts.NewErrorAcknowledgement in favour of channeltypes.NewErrorAcknowledgement

…struct-error-acks-with-abci-codes-&-string-const

seantking · 2022-06-22T13:39:49Z

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -1,9 +1,11 @@
 package controller

 import (
+	"fmt"


nit: format imports

seantking · 2022-06-22T13:51:59Z

modules/core/04-channel/types/acknowledgement_test.go

-import "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
+import (
+	"fmt"
+	"github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"


nit: format imports

codecov-commenter · 2022-06-22T17:08:34Z

Codecov Report

Merging #1565 (230a219) into main (e7d5bda) will decrease coverage by 0.26%.
The diff coverage is 53.42%.

@@            Coverage Diff             @@
##             main    #1565      +/-   ##
==========================================
- Coverage   80.33%   80.06%   -0.27%     
==========================================
  Files         166      165       -1     
  Lines       12346    12381      +35     
==========================================
- Hits         9918     9913       -5     
- Misses       1963     2000      +37     
- Partials      465      468       +3

Impacted Files	Coverage Δ
...27-interchain-accounts/controller/keeper/events.go	`0.00% <0.00%> (ø)`
.../apps/27-interchain-accounts/host/keeper/events.go	`0.00% <0.00%> (ø)`
modules/core/02-client/keeper/proposal.go	`86.36% <50.00%> (-3.81%)`	⬇️
modules/apps/transfer/ibc_module.go	`63.22% <58.82%> (-1.16%)`	⬇️
...7-interchain-accounts/controller/ibc_middleware.go	`84.48% <100.00%> (+0.84%)`	⬆️
...les/apps/27-interchain-accounts/host/ibc_module.go	`100.00% <100.00%> (ø)`
modules/apps/29-fee/keeper/events.go	`100.00% <100.00%> (ø)`
modules/core/02-client/keeper/events.go	`100.00% <100.00%> (ø)`
modules/core/04-channel/types/acknowledgement.go	`90.90% <100.00%> (-9.10%)`	⬇️
modules/apps/transfer/types/codec.go	`55.55% <0.00%> (-44.45%)`	⬇️
... and 2 more

…struct-error-acks-with-abci-codes-&-string-const

crodriguezvega

Good work so far, @chatton! This is a tricky issue, so I think there are a couple of things that still need some more work...

crodriguezvega · 2022-06-22T20:03:22Z

modules/apps/transfer/types/ack_test.go

-
-// TestAcknowledgementError will verify that only a constant string and
-// ABCI error code are used in constructing the acknowledgement error string
-func (suite *TypesTestSuite) TestAcknowledgementError() {


Should we keep one of these two TestAcknowledgementError tests and move it to 04-channel?

Yes, I think the TestABCICodeDeterminism test should also be moved to 04-channel, its duplicated both above here and in ICA tests

crodriguezvega · 2022-06-22T20:24:32Z

modules/apps/27-interchain-accounts/events/emit_events.go

+)
+
+// EmitAcknowledgementErrorEvent emits an acknowledgement error event.
+func EmitAcknowledgementErrorEvent(ctx sdk.Context, err error) {


There is already a EmitAcknowledgementEvent in the host's keeper package. Should we get rid of that one and use this one instead both for the controller and host?

However, in the other modules the event emission functions are all part of the keeper package, so having an events package is deviating from the standard in the codebase right now. Not saying that it's bad, but just raising it here to discuss.

Looking now at the EmitAcknowledgementEvent in the host's keeper package maybe it would be also good to add some more attributes to the event, similar to these:

sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()), sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),

modules/apps/transfer/ibc_module.go

damiannolan · 2022-06-22T21:07:15Z

modules/apps/27-interchain-accounts/events/emit_events.go

+import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
+)


Could we rename this file to events.go to be consistent with other events in other pkgs, also should be in keeper pkg, right?

mega nit: separate imports here also.

Suggested change

import (

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"

)

import (

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"

)

…struct-error-acks-with-abci-codes-&-string-const

chatton · 2022-06-23T11:07:09Z

modules/apps/27-interchain-accounts/controller/keeper/events.go

@@ -0,0 +1,27 @@
+package keeper


for now I duplicated controller/keeper/events.go to avoid calling into a different keeper package and also to not break current conventions.

…ling.

…struct-error-acks-with-abci-codes-&-string-const

crodriguezvega

I think we need a few more changes, @chatton!

modules/apps/transfer/ibc_module.go

crodriguezvega · 2022-06-24T07:20:51Z

modules/apps/27-interchain-accounts/controller/keeper/events.go

+			icatypes.EventTypePacket,
+			sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName),
+			sdk.NewAttribute(icatypes.AttributeKeyAckError, errorMsg),
+			sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()),


Since this is the controller module, I think you should have a new attribute key (i.e. AttributeKeyControllerChannelID).

ah thanks I missed that!

crodriguezvega · 2022-06-24T07:22:07Z

modules/apps/transfer/ibc_module.go

+	}
+
+	if ackErr != nil {
+		eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error()))


I like this!

crodriguezvega · 2022-06-24T07:23:08Z

modules/apps/27-interchain-accounts/controller/keeper/events.go

+		sdk.NewEvent(
+			icatypes.EventTypePacket,
+			sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName),
+			sdk.NewAttribute(icatypes.AttributeKeyAckError, errorMsg),


Do you also want to conditionally add this error string only if err != nil? I think I like the way you did it in transfer.

Maybe you can even also do it in the existing Emit... function in the host module.

yeah makes sense!

crodriguezvega · 2022-06-24T07:26:14Z

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

+	err := fmt.Errorf("cannot receive packet on controller chain")
+	ack := channeltypes.NewErrorAcknowledgement(err)
+	keeper.EmitAcknowledgementEvent(ctx, packet, ack, err)
+	return channeltypes.NewErrorAcknowledgement(err)


Suggested change

return channeltypes.NewErrorAcknowledgement(err)

return ack

modules/apps/27-interchain-accounts/controller/keeper/events.go

crodriguezvega

Thank you for your persistence with this PR, @chatton!

There is one thing that I just realized that we are missing: we need to add to the v3-to-v4 migration docs a short explanation for the signature change of channeltypes.NewErrorAcknowledgement. I think you can add it in this section here.

crodriguezvega · 2022-06-26T12:26:02Z

modules/apps/transfer/ibc_module.go

+		ack = channeltypes.NewErrorAcknowledgement(ackErr)
+	}
+
+	eventAttributes := []sdk.Attribute{


Would it make sense to move this declaration/initialization a bit down, just before the check if ackErr != nil? Just for the purpose of code hygiene...

…struct-error-acks-with-abci-codes-&-string-const

docs/migrations/v3-to-v4.md

modules/apps/27-interchain-accounts/controller/keeper/events.go

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

…struct-error-acks-with-abci-codes-&-string-const

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

…struct-error-acks-with-abci-codes-&-string-const

crodriguezvega

🚀

…struct-error-acks-with-abci-codes-&-string-const

damiannolan

Nice work! LGTM 🚀

CHANGELOG.md

…struct-error-acks-with-abci-codes-&-string-const

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

AdityaSripal

Looks good, thanks @chatton ! My question is for the errors in transfer and ICA, shouldn't we maintain the current standard of wrapping errors with SDK error codes?

cc: @damiannolan @seantking

AdityaSripal · 2022-06-28T10:35:06Z

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -138,7 +140,10 @@ func (im IBCMiddleware) OnRecvPacket(
 	packet channeltypes.Packet,
 	_ sdk.AccAddress,
 ) ibcexported.Acknowledgement {
-	return channeltypes.NewErrorAcknowledgement("cannot receive packet on controller chain")
+	err := fmt.Errorf("cannot receive packet on controller chain")


Should we be creating an SDK error with a code here?

e.g. https://github.com/cosmos/ibc-go/blob/main/modules/apps/transfer/keeper/relay.go#L76

We're getting the error code here and adding it to the string. AFAIK we need to do it this way for determinism?

https://github.com/cosmos/ibc-go/pull/1565/files#diff-7a038c27f47c6c3093e19a350a851ccbc15c5ef7036060e3c631a73579acc333R35

in this particular case the only thing that actually matters is if the error is nil or not, but we use the sdkerrors.Wrap to create this error for consistency.

I think we should use icatypes.ErrInvalidChannelFlow.

See: https://github.com/cosmos/ibc-go/blob/main/modules/apps/27-interchain-accounts/controller/ibc_middleware.go#L79

AdityaSripal · 2022-06-28T10:36:30Z

modules/apps/transfer/ibc_module.go

 	if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
-		ack = channeltypes.NewErrorAcknowledgement("cannot unmarshal ICS-20 transfer packet data")
+		ackErr = fmt.Errorf("cannot unmarshal ICS-20 transfer packet data")


seantking · 2022-06-28T12:44:12Z

modules/apps/transfer/ibc_module.go

@@ -2,6 +2,7 @@ package transfer

 import (
 	"fmt"
+	icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"


formatting :)

* delete test files and add error to transfer types * review comments

@colin-axner

* docs: msg types for fee middleware (#1572) * fix broken link * fix: rm AllowUpdateAfter... check (#1118) * update code & test * update proto and adr026 * update CHANGELOG * update cli docs * update broken milestone link * updated docs * update re: comments * nits: adding inline comments Co-authored-by: Sean King <sean@seking.dev> * chore: adding module name to incentivized packet events (#1580) * docs: adding events to fee middleware docs (#1578) * adding fee distribution docs for relayer operators * adding validation information and basic cli examples * removing unnecessary whitespace * updating definitions * adding ics29 fee middleware events docs * cleanup Co-authored-by: Sean King <seantking@users.noreply.github.com> * docs: adding End Users section to ics29 docs (#1579) * docs: adding End Users section to ics29 docs * Update docs/middleware/ics29-fee/end-users.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Update docs/middleware/ics29-fee/end-users.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Update docs/middleware/ics29-fee/end-users.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Update docs/middleware/ics29-fee/end-users.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Update docs/middleware/ics29-fee/end-users.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Update docs/middleware/ics29-fee/end-users.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Update docs/middleware/ics29-fee/end-users.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Update docs/middleware/ics29-fee/end-users.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Update docs/middleware/ics29-fee/end-users.md Co-authored-by: Damian Nolan <damiannolan@gmail.com> * chore: add link Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * feat: emitting an event when handling a client upgrade proposal (#1570) * feat: emitting an event when handling a client upgrade proposal * refactor: only emit event if err is nil * refactor: idiotmatic go: * docs: nits (#1595) * docs: document that version string can be empty as argument of RegisterInterchainAccount (#1582) Co-authored-by: Carlos Rodriguez <crodveg@gmail.com> * docs: add upgrade client proposal event (#1596) * Consolidate usage of NewErrorAcknowledgement (#1565) * docs: adding line about module accounts / invariants (#1597) * docs: adding line about module accounts / invariants * Update docs/middleware/ics29-fee/fee-distribution.md Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Update docs/middleware/ics29-fee/fee-distribution.md Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> * follow up nits to #1565 (#1598) * delete test files and add error to transfer types * review comments * build(deps): bump github.com/stretchr/testify from 1.7.5 to 1.8.0 (#1616) Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.7.5 to 1.8.0. - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.7.5...v1.8.0) --- updated-dependencies: - dependency-name: github.com/stretchr/testify dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: bump go package version to v4 (#1564) * chore: bump go package version to v4 * update go.mod * fix alignment * fix build * review comments * build fixes * deps: bumping go version 1.18 (#1627) * bumping go version 1.18 * updating broken workflow setup * add backport to v4.0.x and remove backports to v1 (EoL) (#1629) * delete unused 04-channel version functions (#1636) * delete unused code and associated tests * Update CHANGELOG.md * build(deps): bump github.com/cosmos/cosmos-sdk from 0.45.5 to 0.45.6 (#1615) * build(deps): bump github.com/cosmos/cosmos-sdk from 0.45.5 to 0.45.6 Bumps [github.com/cosmos/cosmos-sdk](https://github.com/cosmos/cosmos-sdk) from 0.45.5 to 0.45.6. - [Release notes](https://github.com/cosmos/cosmos-sdk/releases) - [Changelog](https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md) - [Commits](cosmos/cosmos-sdk@v0.45.5...v0.45.6) --- updated-dependencies: - dependency-name: github.com/cosmos/cosmos-sdk dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * Update CHANGELOG.md * copying part of codeql workflow to try to make it pass Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Carlos Rodriguez <crodveg@gmail.com> * update mergify.yml with new release branches (#1654) * Script to dynamically generate list of e2e tests (E2E #1) (#1644) * Add GitHub actions for e2e tests (E2E #2) (#1646) * Remove crossings hello (#1317) * remove crossing hellos from ChanOpenTry * remove crossing hellos testcase * revert single connectionHops check * add comment in MsgChannelOpenTry tx proto about deprecate field * Update proto/ibc/core/channel/v1/tx.proto Co-authored-by: Aditya <adityasripal@gmail.com> * minor fixes && UPDATE CHANGELOG.md * Update proto/ibc/core/channel/v1/tx.proto Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Update proto/ibc/core/channel/v1/tx.proto Co-authored-by: Damian Nolan <damiannolan@gmail.com> * apply remaining changes for crossing hello removal Deprecate previous channel id with proto tag Remove unnecessary test cases from 04-channel Remove crossing hello check in transfer application and the associated test case * remove previous channel check in WriteChannelOpenTry * regenerate proto files * update documentation * add migration documentation * remove unnecessary doc * remove crossing hello notion from ChanOpenAck * apply review suggestions Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Jacob Gadikian <jacobgadikian@gmail.com> Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com> * docs: update roadmap (#1678) * update roadmap * update roadmap * Update roadmap.md Co-authored-by: Carlos Rodriguez <crodveg@gmail.com> * refactor: remove crossing hellos from 03-connection (#1672) * remove crossing hello check from ConnOpenTry and ConnOpenAck * remove unnecessary test cases and fix build * fix tests and add migration docs * fix connection version check in conn open ack * add changelog entry * Update modules/core/03-connection/keeper/handshake.go Co-authored-by: Aditya <adityasripal@gmail.com> * remove unnecessary testing function * improve migration documentation as per review suggestion Co-authored-by: Aditya <adityasripal@gmail.com> * (core/23-commitment/types) doc: fix typo (#1694) * remove spurious `TestABCICodeDeterminism` tests (#1695) ## Description Thanks @colin-axner for noticing this. closes: #XXXX --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [x] Re-reviewed `Files changed` in the Github PR explorer - [ ] Review `Codecov Report` in the comment section below once CI passes * update bug report issue template (#1693) * fix codeowners for 02-client (#1696) ## Description Thanks again @colin-axner for signaling this! closes: #XXXX --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [x] Re-reviewed `Files changed` in the Github PR explorer - [ ] Review `Codecov Report` in the comment section below once CI passes * build(deps): bump google.golang.org/grpc from 1.47.0 to 1.48.0 (#1699) Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.47.0 to 1.48.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.47.0...v1.48.0) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * docs: update middleware documentation (#1639) * docs: update middleware documentation * remove old text and add func keyword * alignment * fix alignment * Update develop.md * review comments * remove empty line * docs: add links in middleware docs to fee middleware implementation (#1641) * docs: add links in middleware docs to fee middleware implementation * add extra line for better readability * fix typos * Adding github action to run goimports (#1673) * Add E2ETestSuite Type (E2E #3) (#1650) * Separate go mod for e2e (E2E #4) (#1701) * put back module name in event (#1681) * fix typo * Extracting e2e tests into two separate workflows (#1719) * add categories * Add fee middleware test suite functions (E2E #5) (#1710) * Build local image to run tests with make e2e-test (#1722) * Remove leftover crossing hello tests in connection handshake (#1724) * remove leftover crossing hello tests in connection handshake * fix bug in tests * chore: adding environment variable to specify go relayer image (#1727) * Thomas/1584 update docs apps (#1675) * restructure content according to outline, fix image and syntax highlighting, fix titles and prepare for content updates * rewrite bind port section * restructure applications doc into folder structure * add keeper section, make some minor corrections in bind ports, custom packet and implmenent IBC module sections * update ibcmodule interface to encorporate the simpliefied handshake callbacks and version negotiation * fix broken links * fix remaining broken link * fix some nits, correct for removal of crossing hellos and add some more explanation on portIDs * update middleware docs to resolve merge confilicts * update fee mw docs, add formating, fix typos, increase readability (#1665) * update fee mw docs, add formating, fix typos, increase readability * fix broken link * Apply suggestions from code review (De)capitalize headings and references to headings for consistency Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * resolving merge conflict * split the CLI commands and small typo correction Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * chore: remove @fedekunze from CODEOWNERS (#1733) * Test for AsyncSingleSender (E2E #6) (#1682) * fix: running e2e-fork for dependabot PRs (#1745) * Adding manual triggering of e2e via workflow dispatch (#1749) * chore: denom traces migration handler (#1680) * update code & test * register migrator service * add changelog entry for #1680 * add issue templates for epics and releases (#1702) * add issue templates for epics and releases * remove text Co-authored-by: Carlos Rodriguez <crodveg@gmail.com> * fix typo * fix typo * feat: allow governance to update the TrustingPeriod of the 07-tendermint light client (#1713) * initial commit * format imports * update docs * update CHANGELOG * update upgrade dev docs * update re: pr comments * E2E Test: TestMsgPayPacketFeeSingleSender (#1756) * move entry to right place * build(deps): bump docker/build-push-action from 3.0.0 to 3.1.0 (#1743) Bumps [docker/build-push-action](https://github.com/docker/build-push-action) from 3.0.0 to 3.1.0. - [Release notes](https://github.com/docker/build-push-action/releases) - [Commits](docker/build-push-action@e551b19...1cb9d22) --- updated-dependencies: - dependency-name: docker/build-push-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * chore: Update makefile (#1770) * remove unused tools, remove incorrect import formatting * uncomment command in makefile * add version matrix (#1767) * add pseudocode to handle empty version string in OnChanOpenInit * fix broken link (#1776) * add item to update the version matrix after a release (#1775) * E2E Test: TestMsgPayPacketFeeSingleSenderTimesOut (#1751) * Move scripts from .github directory into cmd (#1787) Co-authored-by: Charly <charly@interchain.berlin> Co-authored-by: Sean King <sean@seking.dev> Co-authored-by: Sean King <seantking@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Carlos Rodriguez <crodveg@gmail.com> Co-authored-by: Cian Hatton <cian@interchain.io> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: vuong <56973102+vuong177@users.noreply.github.com> Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: Jacob Gadikian <jacobgadikian@gmail.com> Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: rene <41963722+renaynay@users.noreply.github.com> Co-authored-by: tmsdkeys <98807841+tmsdkeys@users.noreply.github.com> Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

chatton and others added 4 commits June 14, 2022 10:43

feat: include ABCI code in failed acknowledgement.

c264fa0

fix: removing transfertypes.NewErrorAcknowledgement and interchainacc…

1ad5de5

…ounts.NewErrorAcknowledgement in favour of channeltypes.NewErrorAcknowledgement

chore: added PR number to CHANGELOG.md

e49b35f

Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…

8283fbe

…struct-error-acks-with-abci-codes-&-string-const

chatton marked this pull request as ready for review June 22, 2022 11:39

chatton requested review from seantking, colin-axner, AdityaSripal, damiannolan, fedekunze, crodriguezvega and charleenfei as code owners June 22, 2022 11:39

seantking reviewed Jun 22, 2022

View reviewed changes

chore: fixed imports

2fd3b3a

Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…

38c89eb

…struct-error-acks-with-abci-codes-&-string-const

crodriguezvega reviewed Jun 22, 2022

View reviewed changes

damiannolan reviewed Jun 22, 2022

View reviewed changes

chatton and others added 2 commits June 23, 2022 11:09

Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…

2102783

…struct-error-acks-with-abci-codes-&-string-const

chore: addressing PR feedback

a5d118e

chatton commented Jun 23, 2022

View reviewed changes

chatton and others added 2 commits June 23, 2022 12:15

fix: displaying ack error message only if there is an error unmarshal…

647d1e1

…ling.

Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…

0cd0af3

…struct-error-acks-with-abci-codes-&-string-const

crodriguezvega requested changes Jun 24, 2022

View reviewed changes

chore: addressed PR feedback

ea311bd

crodriguezvega reviewed Jun 26, 2022

View reviewed changes

modules/apps/27-interchain-accounts/controller/keeper/events.go Show resolved Hide resolved

crodriguezvega reviewed Jun 26, 2022

View reviewed changes

chatton and others added 2 commits June 27, 2022 08:39

chore: fixing formatting, adding changes to migration doc.

8e5eb6e

Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…

725ef86

…struct-error-acks-with-abci-codes-&-string-const

damiannolan reviewed Jun 27, 2022

View reviewed changes

docs/migrations/v3-to-v4.md Show resolved Hide resolved

damiannolan reviewed Jun 27, 2022

View reviewed changes

modules/apps/27-interchain-accounts/controller/keeper/events.go Outdated Show resolved Hide resolved

chatton and others added 4 commits June 27, 2022 17:04

Update modules/apps/27-interchain-accounts/controller/keeper/events.go

52ede1a

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…

e0a953f

…struct-error-acks-with-abci-codes-&-string-const

Update docs/migrations/v3-to-v4.md

c480718

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…

d1089b9

…struct-error-acks-with-abci-codes-&-string-const

crodriguezvega approved these changes Jun 27, 2022

View reviewed changes

Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…

230a219

…struct-error-acks-with-abci-codes-&-string-const

seantking approved these changes Jun 28, 2022

View reviewed changes

damiannolan approved these changes Jun 28, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

chatton and others added 2 commits June 28, 2022 10:31

Merge branch 'main' into cian/issue#819-add-a-generic-function-to-con…

6c15291

…struct-error-acks-with-abci-codes-&-string-const

Update CHANGELOG.md

84d2d44

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

AdityaSripal approved these changes Jun 28, 2022

View reviewed changes

chore: wrapping errors using sdkerrors.Wrapf

d3e72ea

seantking suggested changes Jun 28, 2022

View reviewed changes

chore: fixing imports

50dfd89

seantking approved these changes Jun 28, 2022

View reviewed changes

chatton merged commit b40dbc6 into main Jun 28, 2022

chatton deleted the cian/issue#819-add-a-generic-function-to-construct-error-acks-with-abci-codes-&-string-const branch June 28, 2022 13:57

crodriguezvega added a commit that referenced this pull request Jun 29, 2022

follow up nits to #1565 (#1598)

ecf3543

* delete test files and add error to transfer types * review comments

This was referenced Jul 10, 2022

put back module name in fungible_token_packet event #1681

Merged

Unify Recv/Ack fungible_token_packet event #1254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate usage of NewErrorAcknowledgement #1565

Consolidate usage of NewErrorAcknowledgement #1565

chatton commented Jun 22, 2022 •

edited

Loading

seantking Jun 22, 2022

seantking Jun 22, 2022

codecov-commenter commented Jun 22, 2022 •

edited

Loading

crodriguezvega left a comment

crodriguezvega Jun 22, 2022

damiannolan Jun 22, 2022

crodriguezvega Jun 22, 2022

damiannolan Jun 22, 2022 •

edited

Loading

chatton Jun 23, 2022

crodriguezvega left a comment

crodriguezvega Jun 24, 2022

chatton Jun 24, 2022

crodriguezvega Jun 24, 2022

crodriguezvega Jun 24, 2022

chatton Jun 24, 2022

crodriguezvega Jun 24, 2022

crodriguezvega left a comment

crodriguezvega Jun 26, 2022 •

edited

Loading

crodriguezvega left a comment

damiannolan left a comment

AdityaSripal left a comment

AdityaSripal Jun 28, 2022

AdityaSripal Jun 28, 2022

seantking Jun 28, 2022

chatton Jun 28, 2022

damiannolan Jun 28, 2022

AdityaSripal Jun 28, 2022

seantking Jun 28, 2022

Consolidate usage of NewErrorAcknowledgement #1565

Consolidate usage of NewErrorAcknowledgement #1565

Conversation

chatton commented Jun 22, 2022 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 22, 2022 • edited Loading

Codecov Report

crodriguezvega 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

damiannolan Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega 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

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

crodriguezvega Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

AdityaSripal 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Jun 22, 2022 •

edited

Loading

codecov-commenter commented Jun 22, 2022 •

edited

Loading

damiannolan Jun 22, 2022 •

edited

Loading

crodriguezvega Jun 26, 2022 •

edited

Loading