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

Jump to bottom

[CT-1262] add e2e tests for new auth flow failure cases #2461

Merged
merged 6 commits into from
Oct 8, 2024
Merged

[CT-1262] add e2e tests for new auth flow failure cases #2461

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c65e3b9
[CT-1262] add e2e tests for new auth flow failure cases
jayy04 Oct 3, 2024
d1d3aff
more tests
jayy04 Oct 3, 2024
39d168d
fix test
jayy04 Oct 3, 2024
1ed9994
Merge branch 'main' into jy/ct-1262
jayy04 Oct 4, 2024
d896e98
comment
jayy04 Oct 4, 2024
28c7013
comemnts
jayy04 Oct 4, 2024
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
13 changes: 10 additions & 3 deletions protocol/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,9 +1233,16 @@ func New(

// Initialize authenticators
app.AuthenticatorManager = authenticator.NewAuthenticatorManager()
app.AuthenticatorManager.InitializeAuthenticators([]accountplusmoduletypes.Authenticator{
authenticator.NewSignatureVerification(app.AccountKeeper),
})
app.AuthenticatorManager.InitializeAuthenticators(
[]accountplusmoduletypes.Authenticator{
authenticator.NewAllOf(app.AuthenticatorManager),
authenticator.NewAnyOf(app.AuthenticatorManager),
authenticator.NewSignatureVerification(app.AccountKeeper),
authenticator.NewMessageFilter(),
authenticator.NewClobPairIdFilter(),
authenticator.NewSubaccountFilter(),
},
)
app.AccountPlusKeeper = *accountplusmodulekeeper.NewKeeper(
appCodec,
keys[accountplusmoduletypes.StoreKey],
Expand Down
6 changes: 5 additions & 1 deletion protocol/testutil/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import (
"github.com/dydxprotocol/v4-chain/protocol/testutil/appoptions"
"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
testlog "github.com/dydxprotocol/v4-chain/protocol/testutil/logger"
aptypes "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
assettypes "github.com/dydxprotocol/v4-chain/protocol/x/assets/types"
blocktimetypes "github.com/dydxprotocol/v4-chain/protocol/x/blocktime/types"
bridgetypes "github.com/dydxprotocol/v4-chain/protocol/x/bridge/types"
Expand Down Expand Up @@ -207,7 +208,8 @@ type GenesisStates interface {
govplus.GenesisState |
vaulttypes.GenesisState |
revsharetypes.GenesisState |
marketmapmoduletypes.GenesisState
marketmapmoduletypes.GenesisState |
aptypes.GenesisState
}

// UpdateGenesisDocWithAppStateForModule updates the supplied genesis doc using the provided function. The function
Expand Down Expand Up @@ -269,6 +271,8 @@ func UpdateGenesisDocWithAppStateForModule[T GenesisStates](genesisDoc *types.Ge
moduleName = marketmapmoduletypes.ModuleName
case listingtypes.GenesisState:
moduleName = listingtypes.ModuleName
case aptypes.GenesisState:
moduleName = aptypes.ModuleName
default:
panic(fmt.Errorf("Unsupported type %T", t))
}
Expand Down
8 changes: 8 additions & 0 deletions protocol/testutil/constants/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,14 @@ const GenesisState = `{
"validator_historical_rewards": [],
"validator_slash_events": []
},
"dydxaccountplus": {
"accounts": [],
"params": {
"is_smart_account_active": false
},
"next_authenticator_id": "0",
"authenticator_data": []
},
"epochs": {
"epoch_info_list": [
{
Expand Down
7 changes: 5 additions & 2 deletions protocol/x/accountplus/authenticator/all_of.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (aoa AllOf) StaticGas() uint64 {
}

func (aoa AllOf) Initialize(config []byte) (types.Authenticator, error) {
var initDatas []SubAuthenticatorInitData
var initDatas []types.SubAuthenticatorInitData
if err := json.Unmarshal(config, &initDatas); err != nil {
return nil, errorsmod.Wrap(err, "failed to parse sub-authenticators initialization data")
}
Expand Down Expand Up @@ -99,7 +99,10 @@ func (aoa AllOf) Authenticate(ctx sdk.Context, request types.AuthenticationReque
request.Signature = signatures[i]
}
if err := auth.Authenticate(ctx, request); err != nil {
return err
return errorsmod.Wrap(
types.ErrAllOfVerification,
err.Error(),
)
}
}
return nil
Expand Down
4 changes: 2 additions & 2 deletions protocol/x/accountplus/authenticator/any_of.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func (aoa AnyOf) StaticGas() uint64 {

func (aoa AnyOf) Initialize(config []byte) (types.Authenticator, error) {
// Decode the initialization data for each sub-authenticator
var initDatas []SubAuthenticatorInitData
var initDatas []types.SubAuthenticatorInitData
if err := json.Unmarshal(config, &initDatas); err != nil {
return nil, errorsmod.Wrap(err, "failed to parse sub-authenticators initialization data")
}
Expand Down Expand Up @@ -126,7 +126,7 @@ func (aoa AnyOf) Authenticate(ctx sdk.Context, request types.AuthenticationReque
if err != nil {
// return all errors
return errorsmod.Wrapf(
sdkerrors.ErrUnauthorized,
types.ErrAnyOfVerification,
"all sub-authenticators failed to authenticate: %s",
strings.Join(subAuthErrors, "; "),
)
Expand Down
3 changes: 1 addition & 2 deletions protocol/x/accountplus/authenticator/clob_pair_id_filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (

errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
)
Expand Down Expand Up @@ -77,7 +76,7 @@ func (m ClobPairIdFilter) Authenticate(ctx sdk.Context, request types.Authentica
for _, clobPairId := range requestOrderIds {
if _, ok := m.whitelist[clobPairId]; !ok {
return errorsmod.Wrapf(
sdkerrors.ErrUnauthorized,
types.ErrClobPairIdVerification,
"order id %d not in whitelist %v",
clobPairId,
m.whitelist,
Expand Down
4 changes: 2 additions & 2 deletions protocol/x/accountplus/authenticator/clob_pair_id_filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import (

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"

"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/authenticator"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/lib"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"

"github.com/stretchr/testify/suite"
)
Expand Down Expand Up @@ -105,7 +105,7 @@ func (s *ClobPairIdFilterTest) TestFilter() {
if tt.match {
s.Require().NoError(err)
} else {
s.Require().ErrorIs(err, sdkerrors.ErrUnauthorized)
s.Require().ErrorIs(err, types.ErrClobPairIdVerification)
}
})
}
Expand Down
9 changes: 2 additions & 7 deletions protocol/x/accountplus/authenticator/composite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ import (
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
)

type SubAuthenticatorInitData struct {
Type string `json:"type"`
Config []byte `json:"config"`
}

func subTrack(
ctx sdk.Context,
request types.AuthenticationRequest,
Expand Down Expand Up @@ -50,7 +45,7 @@ func onSubAuthenticatorsAdded(
authenticatorId string,
am *AuthenticatorManager,
) error {
var initDatas []SubAuthenticatorInitData
var initDatas []types.SubAuthenticatorInitData
if err := json.Unmarshal(data, &initDatas); err != nil {
return errorsmod.Wrapf(err, "failed to unmarshal sub-authenticator init data")
}
Expand Down Expand Up @@ -97,7 +92,7 @@ func onSubAuthenticatorsRemoved(
authenticatorId string,
am *AuthenticatorManager,
) error {
var initDatas []SubAuthenticatorInitData
var initDatas []types.SubAuthenticatorInitData
if err := json.Unmarshal(data, &initDatas); err != nil {
return err
}
Expand Down
20 changes: 10 additions & 10 deletions protocol/x/accountplus/authenticator/composition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,9 @@ func (s *AggregatedAuthenticatorsTest) TestAnyOf() {
for _, tc := range testCases {
s.T().Run(tc.name, func(t *testing.T) {
// Convert the authenticators to InitializationData
initData := []authenticator.SubAuthenticatorInitData{}
initData := []types.SubAuthenticatorInitData{}
for _, auth := range tc.authenticators {
initData = append(initData, authenticator.SubAuthenticatorInitData{
initData = append(initData, types.SubAuthenticatorInitData{
Type: auth.Type(),
Config: testData,
})
Expand Down Expand Up @@ -344,9 +344,9 @@ func (s *AggregatedAuthenticatorsTest) TestAllOf() {
for _, tc := range testCases {
s.T().Run(tc.name, func(t *testing.T) {
// Convert the authenticators to InitializationData
initData := []authenticator.SubAuthenticatorInitData{}
initData := []types.SubAuthenticatorInitData{}
for _, auth := range tc.authenticators {
initData = append(initData, authenticator.SubAuthenticatorInitData{
initData = append(initData, types.SubAuthenticatorInitData{
Type: auth.Type(),
Config: testData,
})
Expand Down Expand Up @@ -562,29 +562,29 @@ func (csa *CompositeSpyAuth) buildInitData() ([]byte, error) {
}
return json.Marshal(spyData)
} else if len(csa.anyOf) > 0 {
var initData []authenticator.SubAuthenticatorInitData
var initData []types.SubAuthenticatorInitData
for _, subAuth := range csa.anyOf {
data, err := subAuth.buildInitData()
if err != nil {
return nil, err
}

initData = append(initData, authenticator.SubAuthenticatorInitData{
initData = append(initData, types.SubAuthenticatorInitData{
Type: subAuth.Type(),
Config: data,
})
}

return json.Marshal(initData)
} else if len(csa.allOf) > 0 {
var initData []authenticator.SubAuthenticatorInitData
var initData []types.SubAuthenticatorInitData
for _, subAuth := range csa.allOf {
data, err := subAuth.buildInitData()
if err != nil {
return nil, err
}

initData = append(initData, authenticator.SubAuthenticatorInitData{
initData = append(initData, types.SubAuthenticatorInitData{
Type: subAuth.Type(),
Config: data,
})
Expand Down Expand Up @@ -901,14 +901,14 @@ func (s *AggregatedAuthenticatorsTest) TestAnyOfNotWritingFailedSubAuthState() {
}

func marshalAuth(ta testAuth, testData []byte) ([]byte, error) {
initData := []authenticator.SubAuthenticatorInitData{}
initData := []types.SubAuthenticatorInitData{}

for _, sub := range ta.subAuths {
subData, err := marshalAuth(sub, testData)
if err != nil {
return nil, err
}
initData = append(initData, authenticator.SubAuthenticatorInitData{
initData = append(initData, types.SubAuthenticatorInitData{
Type: sub.authenticator.Type(),
Config: subData,
})
Expand Down
3 changes: 1 addition & 2 deletions protocol/x/accountplus/authenticator/message_filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
)

Expand Down Expand Up @@ -54,7 +53,7 @@ func (m MessageFilter) Track(ctx sdk.Context, request types.AuthenticationReques
func (m MessageFilter) Authenticate(ctx sdk.Context, request types.AuthenticationRequest) error {
if _, ok := m.whitelist[sdk.MsgTypeURL(request.Msg)]; !ok {
return errorsmod.Wrapf(
sdkerrors.ErrUnauthorized,
types.ErrMessageTypeVerification,
"message types do not match. Got %s, Expected %v",
sdk.MsgTypeURL(request.Msg),
m.whitelist,
Expand Down
4 changes: 2 additions & 2 deletions protocol/x/accountplus/authenticator/message_filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ import (

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
bank "github.com/cosmos/cosmos-sdk/x/bank/types"
clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
satypes "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"

"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/authenticator"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/lib"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"

"github.com/stretchr/testify/suite"
)
Expand Down Expand Up @@ -121,7 +121,7 @@ func (s *MessageFilterTest) TestBankSend() {
if tt.match {
s.Require().NoError(err)
} else {
s.Require().ErrorIs(err, sdkerrors.ErrUnauthorized)
s.Require().ErrorIs(err, types.ErrMessageTypeVerification)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion protocol/x/accountplus/authenticator/signature_authenticator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (sva SignatureVerification) Authenticate(ctx sdk.Context, request types.Aut

if !sva.PubKey.VerifySignature(request.SignModeTxData.Direct, request.Signature) {
return errorsmod.Wrapf(
sdkerrors.ErrUnauthorized,
types.ErrSignatureVerification,
"signature verification failed; please verify account number (%d), sequence (%d) and chain-id (%s)",
request.TxData.AccountNumber,
request.TxData.AccountSequence,
Expand Down
3 changes: 1 addition & 2 deletions protocol/x/accountplus/authenticator/subaccount_filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (

errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
)
Expand Down Expand Up @@ -75,7 +74,7 @@ func (m SubaccountFilter) Authenticate(ctx sdk.Context, request types.Authentica
for _, subaccountNum := range requestSubaccountNums {
if _, ok := m.whitelist[subaccountNum]; !ok {
return errorsmod.Wrapf(
sdkerrors.ErrUnauthorized,
types.ErrSubaccountVerification,
"subaccount number %d not in whitelist %v",
subaccountNum,
m.whitelist,
Expand Down
4 changes: 2 additions & 2 deletions protocol/x/accountplus/authenticator/subaccount_filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import (

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"

"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/authenticator"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/lib"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"

"github.com/stretchr/testify/suite"
)
Expand Down Expand Up @@ -105,7 +105,7 @@ func (s *SubaccountFilterTest) TestFilter() {
if tt.match {
s.Require().NoError(err)
} else {
s.Require().ErrorIs(err, sdkerrors.ErrUnauthorized)
s.Require().ErrorIs(err, types.ErrSubaccountVerification)
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Action Required: Inconsistent Usage of Error Types

While the change from sdkerrors.ErrUnauthorized to types.ErrSubaccountVerification enhances error specificity, there are remaining instances of sdkerrors.ErrUnauthorized across the codebase that may need to be updated for consistency.

Please review and address the following files to ensure uniform error handling:

  • protocol/x/ratelimit/util/ibc.go
  • protocol/x/subaccounts/keeper/subaccount_test.go
  • protocol/x/subaccounts/keeper/transfer_test.go
  • protocol/x/sending/e2e/isolated_subaccount_transfers_test.go
  • protocol/x/clob/keeper/liquidations_test.go
  • protocol/x/clob/e2e/isolated_subaccount_orders_test.go
  • protocol/x/clob/ante/clob_test.go
  • protocol/x/clob/ante/clob.go
  • protocol/testing/e2e/authz/authz_test.go
  • protocol/x/accountplus/testutils/spy_authenticator.go
  • protocol/x/accountplus/testutils/generic_authenticator.go
  • protocol/x/accountplus/lib/authentication_request.go
  • protocol/x/accountplus/keeper/authenticators.go
  • protocol/x/accountplus/keeper/msg_server.go
  • protocol/x/accountplus/ante/timestampnonce.go
  • protocol/x/accountplus/authenticator/signature_authenticator.go
  • protocol/x/accountplus/authenticator/composite.go
  • protocol/x/accountplus/authenticator/any_of.go
  • protocol/x/accountplus/authenticator/all_of.go
  • protocol/x/accountplus/ante/ante.go
  • protocol/app/msgs/app_test.go
  • protocol/app/ante.go
  • protocol/app/ante/gas_test.go
  • protocol/app/ante/sigverify.go
  • protocol/app/ante/msg_type_test.go
  • protocol/app/ante/replay_protection.go
  • protocol/app/ante/msg_type.go
  • protocol/app/ante/basic_test.go
  • protocol/app/ante/market_update.go

Ensuring that all instances are consistently updated will maintain code quality and prevent potential errors.

🔗 Analysis chain

LGTM: Error type change improves specificity.

The change from sdkerrors.ErrUnauthorized to types.ErrSubaccountVerification is an improvement as it provides a more specific error type for subaccount verification failures. This aligns well with best practices for error handling.

To ensure consistency across the codebase, please run the following script:

This script will help identify:

  1. Where the new ErrSubaccountVerification is being used.
  2. If there are any remaining uses of ErrUnauthorized that might need to be updated.
  3. Any other files using sdkerrors that might need review.

Please review the results to ensure consistency across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of ErrSubaccountVerification and potential remaining uses of ErrUnauthorized

# Test 1: Check for uses of ErrSubaccountVerification
echo "Occurrences of ErrSubaccountVerification:"
rg --type go "ErrSubaccountVerification"

# Test 2: Check for any remaining uses of ErrUnauthorized
echo "\nRemaining occurrences of ErrUnauthorized:"
rg --type go "ErrUnauthorized"

# Test 3: Check for any files that might need updating
echo "\nPotential files that might need updating:"
rg --type go -l "sdkerrors\.Err.*"

Length of output: 3989

}
})
}
Expand Down
32 changes: 32 additions & 0 deletions protocol/x/accountplus/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,36 @@ var (
6,
"The transaction has multiple signers",
)

// Errors for failing authenticator validation
ErrSignatureVerification = errorsmod.Register(
ModuleName,
100,
"Signature verification failed",
)
ErrMessageTypeVerification = errorsmod.Register(
ModuleName,
101,
"Message type verification failed",
)
ErrClobPairIdVerification = errorsmod.Register(
ModuleName,
102,
"Clob pair id verification failed",
)
ErrSubaccountVerification = errorsmod.Register(
ModuleName,
103,
"Subaccount verification failed",
)
ErrAllOfVerification = errorsmod.Register(
ModuleName,
104,
"AllOf verification failed",
)
ErrAnyOfVerification = errorsmod.Register(
ModuleName,
105,
"AnyOf verification failed",
)
)
5 changes: 5 additions & 0 deletions protocol/x/accountplus/types/iface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ type InitializedAuthenticator struct {
Authenticator Authenticator
}

type SubAuthenticatorInitData struct {
Type string `json:"type"`
Config []byte `json:"config"`
}

// Authenticator is an interface that encapsulates all authentication functionalities essential for
// verifying transactions, paying transaction fees, and managing gas consumption during verification.
type Authenticator interface {
Expand Down
6 changes: 5 additions & 1 deletion protocol/x/accountplus/types/message_add_authenticator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

const AUTHENTICATOR_DATA_MAX_LENGTH = 256
// AUTHENTICATOR_DATA_MAX_LENGTH is the maximum length of the data field in an authenticator.
//
// This is used as a light-weight spam mitigation measure to prevent users from adding
// arbitrarily complex authenticators that are too resource intensive.
const AUTHENTICATOR_DATA_MAX_LENGTH = 1024

// ValidateBasic performs stateless validation for the `MsgAddAuthenticator` msg.
func (msg *MsgAddAuthenticator) ValidateBasic() (err error) {
Expand Down
Loading
Loading