Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(abci): add unwrapper to handle join error #23712

Open
wants to merge 5 commits into
base: release/v0.50.x
Choose a base branch
from
+51 −5
Open

fix(abci): add unwrapper to handle join error #23712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
96aac27
fix(abci): add unwrapper to handle join error
mmsqe Feb 15, 2025
b091ac7
Update baseapp/baseapp_test.go
mmsqe Feb 15, 2025
0f0b958
Merge remote-tracking branch 'origin/release/v0.50.x' into release/v0…
mmsqe Feb 27, 2025
68c0988
cleanup
mmsqe Feb 27, 2025
03762a1
Apply suggestions from code review
mmsqe Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions baseapp/baseapp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"testing"
"time"

"github.com/cockroachdb/errors"
abci "github.com/cometbft/cometbft/abci/types"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
dbm "github.com/cosmos/cosmos-db"
Expand All @@ -32,6 +33,7 @@ import (
"github.com/cosmos/cosmos-sdk/testutil"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
)

Expand Down Expand Up @@ -883,3 +885,26 @@ func TestLoadVersionPruning(t *testing.T) {
require.Nil(t, err)
testLoadVersionHelper(t, app, int64(7), lastCommitID)
}

func TestErrorsJoinAndABCIInfo(t *testing.T) {
tErr := sdkerrors.ErrInsufficientFunds
tErrCode := tErr.ABCICode()
tests := []struct {
name string
err error
exp uint32
}{
{name: "nil", err: nil, exp: 0},
{name: "ErrInsufficientFunds", err: tErr, exp: tErrCode},
{name: "joined ErrInsufficientFunds", err: errors.Join(tErr), exp: tErrCode},
{name: "joined nil ErrInsufficientFunds", err: errors.Join(nil, tErr), exp: tErrCode},
{name: "joined ErrInsufficientFunds nil", err: errors.Join(tErr, nil), exp: tErrCode},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
_, act, _ := errorsmod.ABCIInfo(tc.err, false)
require.Equal(t, tc.exp, act)
})
}
}
4 changes: 4 additions & 0 deletions errors/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

## [Unreleased]

### Bug Fixes

* (abci) [#23709](https://github.com/cosmos/cosmos-sdk/pull/23709) Add unwrapper to handle join error.

### Features

* [#15989](https://github.com/cosmos/cosmos-sdk/pull/15989) Add `ErrStopIterating` for modules to use for breaking out of iteration.
Expand Down
14 changes: 14 additions & 0 deletions errors/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ type coder interface {
ABCICode() uint32
}

type unwrappableError interface {
Unwrap() []error
}

// abciCode tests if given error contains an ABCI code and returns the value of
// it if available. This function is testing for the causer interface as well
// and unwraps the error.
Expand All @@ -59,6 +63,16 @@ func abciCode(err error) uint32 {
}

for {
if c, ok := err.(unwrappableError); ok {
errs := c.Unwrap()
for _, e := range errs {
if e != nil {
err = e
Copy link
Contributor

@alpe alpe Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if "first error" wins is correct. You could also have first coder error win. or last... it would be good to understand the scenario better. I have commented on the issue.
Skipping any nil errors does make sense though. This could be achieved by using Go stdlib errors.Join in baseapp as well.

break
}
}
}

if c, ok := err.(coder); ok {
return c.ABCICode()
}
Expand Down
2 changes: 2 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ replace (
github.com/syndtr/goleveldb => github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7
)

replace cosmossdk.io/errors => ./errors

retract (
// false start by tagging the wrong branch
v0.50.0
Expand Down
2 changes: 0 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ cosmossdk.io/core v0.11.0 h1:vtIafqUi+1ZNAE/oxLOQQ7Oek2n4S48SWLG8h/+wdbo=
cosmossdk.io/core v0.11.0/go.mod h1:LaTtayWBSoacF5xNzoF8tmLhehqlA9z1SWiPuNC6X1w=
cosmossdk.io/depinject v1.1.0 h1:wLan7LG35VM7Yo6ov0jId3RHWCGRhe8E8bsuARorl5E=
cosmossdk.io/depinject v1.1.0/go.mod h1:kkI5H9jCGHeKeYWXTqYdruogYrEeWvBQCw1Pj4/eCFI=
cosmossdk.io/errors v1.0.1 h1:bzu+Kcr0kS/1DuPBtUFdWjzLqyUuCiyHjyJB6srBV/0=
cosmossdk.io/errors v1.0.1/go.mod h1:MeelVSZThMi4bEakzhhhE/CKqVv3nOJDA25bIqRDu/U=
cosmossdk.io/log v1.4.1 h1:wKdjfDRbDyZRuWa8M+9nuvpVYxrEOwbD/CA8hvhU8QM=
cosmossdk.io/log v1.4.1/go.mod h1:k08v0Pyq+gCP6phvdI6RCGhLf/r425UT6Rk/m+o74rU=
cosmossdk.io/math v1.4.0 h1:XbgExXFnXmF/CccPPEto40gOO7FpWu9yWNAZPN3nkNQ=
Expand Down
9 changes: 6 additions & 3 deletions x/group/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,8 @@ func (s *TestSuite) TestUpdateGroupMembers() {
{
Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
GroupId: groupID,
}},
},
},
},
"remove existing member": {
req: &group.MsgUpdateGroupMembers{
Expand All @@ -424,7 +425,8 @@ func (s *TestSuite) TestUpdateGroupMembers() {
{
Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
GroupId: groupID,
}},
},
},
},
"remove unknown member": {
req: &group.MsgUpdateGroupMembers{
Expand All @@ -447,7 +449,8 @@ func (s *TestSuite) TestUpdateGroupMembers() {
}, {
Member: &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
GroupId: groupID,
}},
},
},
},
"with wrong admin": {
req: &group.MsgUpdateGroupMembers{
Expand Down
Loading