Use standard IBC errors #3908

nicolaslara · 2023-01-03T09:25:17Z

Refs: #3838 (comment)

What is the purpose of the change

Wasm hooks now uses the standard ibc v4 errors but emits the errors for the user to review.

Brief Changelog

(for example:)

Added an utility function to osmoutils for emiting errors before returning an ibc ack error
Updated wasmhooks errors to use that function

Testing and Verifying

Updated tests to check the errors properly (TODO)

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

mattverse

Looks nice! Left some minor comments, please take a look

mattverse · 2023-01-04T09:52:44Z

x/ibc-hooks/types/errors.go

 	ErrBadMetadataFormatMsg = "wasm metadata not properly formatted for: '%v'. %s"
 	ErrBadExecutionMsg      = "cannot execute contract: %v"
-	ErrBadResponse          = "cannot create response: %v"
+
+	ErrMsgValidation = sdkerrors.Register("wasm-hooks", 1, "error in wasmhook message validation")


Just curious why use "wasm-hooks" instead of ModuleName? Also is it possible in this case to avoid using sdkerrors? AFAIK our goal in general is to refrain from using sdk errors

Just curious why use "wasm-hooks" instead of ModuleName?

This error is specific to the validation of the wasm hook. The module could (and probably will in the future) have different hooks. That said, don't mind changing it.

is it possible in this case to avoid using sdkerrors? AFAIK our goal in general is to refrain from using sdk errors

What's the alternative? Ibc internally expects an error with codespace and code id. (https://github.com/cosmos/ibc-go/blob/b40dbc646c7191aaee12d3a70243f9b6f3f80919/modules/core/04-channel/types/acknowledgement.go#L32-L42)

This error is specific to the validation of the wasm hook. The module could (and probably will in the future) have different hooks. That said, don't mind changing it.

Ah I see what you were trying to do. LGTM!

What's the alternative? Ibc internally expects an error with codespace and code id. (https://github.com/cosmos/ibc-go/blob/b40dbc646c7191aaee12d3a70243f9b6f3f80919/modules/core/04-channel/types/acknowledgement.go#L32-L42)

Don't know much about ibc-go but is this the method that receives the error? https://github.com/cosmos/ibc-go/blob/b40dbc646c7191aaee12d3a70243f9b6f3f80919/modules/core/04-channel/types/acknowledgement.go#L32-L35 looks like it takes in go lang error. Or I assume there might be different components in ibc-go that accepts codespace and code-id. If so, please feel free to resolve this comment!

yeah, we can pass a golang errors (the sdk errors are golang errors under the hood). I guess I could define the error type using sdkerrors.New or manually as Error{codespace: codespace, code: code, desc: desc}, but not sure what the advantage of doing that would be.

Maybe we should have a standard way to do this across osmosis?

@ValarDragon can you chime in here? I think we should leave these using sdkerrors. AFAICT, passing a normal error to IBC's channeltypes.NewErrorAcknowledgement will return the default "internalABCICode" (1). Having the code numbers is useful to determine what happened.

We're generally trying to rethink codespacing in cosmos: cosmos/cosmos-sdk#14070

Very few people actually use codespacing in their integrator logic.

I'm not very opinionated on this, since its unlikely these errors are things we'd want to change in a patch release.

I believe we have to start registration from 2 onwards though

Ah given that this is IBC-standard here, agreed with SDK registering these

ok, so, in conclusion: keep sdk errors for this so it matches other IBC errors and move the first code to 2 (which was just an oversight on my part), right?

osmoutils/ibc.go

ValarDragon · 2023-01-06T08:35:34Z

osmoutils/ibc.go

-		},
+// NewEmitErrorAcknowledgement creates a new error acknowledgement after having emitted an event with the
+// details of the error.
+func NewEmitErrorAcknowledgement(ctx sdk.Context, err error, errorContexts ...string) channeltypes.Acknowledgement {


Why don't we just move this to ibc-hooks, then we can delete the entire ibc-hooks <> osmoutils dependency

sounds good!

ahh, no. Just realized it's in osmoutils because rate-limiting is also using it (we could have rate limiting import it from wasm hooks, but it seems dirtier)

Didn't we want rate limiting to import ibc-hooks anyway? (This isn't release blocking at all either way though)

yeah, that makes sense. We can change it later when rate limits is based off ibc hooks.

ValarDragon · 2023-01-06T08:39:53Z

osmoutils/ibc.go

+		),
+	})
+
+	return channeltypes.NewErrorAcknowledgement(err)


cref https://github.com/cosmos/ibc-go/blob/ecb845d5e43f53decf48f8ed88c7847a9a4375cb/modules/core/04-channel/types/acknowledgement.go#L32

This makes sense to me!

ValarDragon

LGTM, we just need to increment codespace by one

* reverted errors to the ibc standard * revision change * updated dependencies * fix rate limit errors * fixed rate limit errors * updated versions * updated commit to after the merge * convert new error * new go mod * increment error codes * updated go mod to latest ibc-hooks Co-authored-by: Dev Ojha <[email protected]>

* Enforce SDK 45 in the dependencies (#3863) * using sdk 45 * added osmoutils to go.sum * move all versions to 0.45 * changed for all * fixed import to latest * updated go.mod * Lifecycle completed without contracts (#3927) * selecting lyfecycle change * updated version and tidy * updated bersions again to one with the proper go.sum * proper counter contract for tests * updated test bytecode * wasm hooks from main * lifecycle message update * go mod tidy * Use standard IBC errors (#3908) * reverted errors to the ibc standard * revision change * updated dependencies * fix rate limit errors * fixed rate limit errors * updated versions * updated commit to after the merge * convert new error * new go mod * increment error codes * updated go mod to latest ibc-hooks Co-authored-by: Dev Ojha <[email protected]> * new commit * update ibc-hook version * Update root version * updated osmomath Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Dev Ojha <[email protected]>

nicolaslara added 4 commits January 3, 2023 10:18

reverted errors to the ibc standard

028dd7c

revision change

d42328e

updated dependencies

4e6afca

fix rate limit errors

5ac89a9

nicolaslara added the V:state/breaking State machine breaking PR label Jan 3, 2023

fixed rate limit errors

17acd94

mattverse reviewed Jan 4, 2023

View reviewed changes

nicolaslara added 6 commits January 4, 2023 16:35

Merge branch 'main' into nicolas/ibc-standard-errors

3aa71af

updated versions

f2d53fe

Merge branch 'main' into nicolas/ibc-standard-errors

bccf520

updated commit to after the merge

114f702

convert new error

1e6fcd9

new go mod

7f9b8c4

nicolaslara mentioned this pull request Jan 5, 2023

Cherry picking remaining PRs for v14.x #3928

Merged

ValarDragon reviewed Jan 6, 2023

View reviewed changes

ValarDragon approved these changes Jan 6, 2023

View reviewed changes

ValarDragon and others added 3 commits January 6, 2023 02:47

Merge branch 'main' into nicolas/ibc-standard-errors

15e5d30

increment error codes

07448a7

updated go mod to latest ibc-hooks

df47862

nicolaslara merged commit 73b5717 into main Jan 6, 2023

nicolaslara deleted the nicolas/ibc-standard-errors branch January 6, 2023 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use standard IBC errors #3908

Use standard IBC errors #3908

nicolaslara commented Jan 3, 2023

mattverse left a comment

mattverse Jan 4, 2023

nicolaslara Jan 4, 2023

mattverse Jan 5, 2023 •

edited

Loading

nicolaslara Jan 5, 2023

nicolaslara Jan 5, 2023

ValarDragon Jan 6, 2023

ValarDragon Jan 6, 2023

ValarDragon Jan 6, 2023

nicolaslara Jan 6, 2023

ValarDragon Jan 6, 2023

ValarDragon Jan 6, 2023

nicolaslara Jan 6, 2023

nicolaslara Jan 6, 2023

ValarDragon Jan 6, 2023

nicolaslara Jan 6, 2023

ValarDragon Jan 6, 2023

ValarDragon left a comment

Use standard IBC errors #3908

Use standard IBC errors #3908

Conversation

nicolaslara commented Jan 3, 2023

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

mattverse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse Jan 5, 2023 • edited Loading

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

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

ValarDragon left a comment

Choose a reason for hiding this comment

mattverse Jan 5, 2023 •

edited

Loading