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.

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

[CT-1262] add e2e tests for permissioned keys success cases #2479

Merged
merged 8 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
38 changes: 14 additions & 24 deletions protocol/app/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,21 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
),
sigVerification: accountplusante.NewCircuitBreakerDecorator(
options.Codec,
accountplusante.NewAuthenticatorDecorator(
options.Codec,
options.AccountplusKeeper,
options.AccountKeeper,
options.SignModeHandler,
sdk.ChainAnteDecorators(
accountplusante.NewAuthenticatorDecorator(
options.Codec,
options.AccountplusKeeper,
options.AccountKeeper,
options.SignModeHandler,
),
),
customante.NewSigVerificationDecorator(
options.AccountKeeper,
options.SignModeHandler,
sdk.ChainAnteDecorators(
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding why was it necessary to move SetPubKey and SigGas under this chain and thus not applicable to smart account flow?

Copy link
Contributor Author

@jayy04 jayy04 Oct 16, 2024

Choose a reason for hiding this comment

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

SetPubKey expects the signer of the message to match the public key (link), and I was getting some errors here.

After moving SetPubKey, I was getting some public key = nil errors in SigGas here because public key is not set. So moved this as well.

This is also how osmosis does it (link)

ante.NewSetPubKeyDecorator(options.AccountKeeper),
ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remind how how gas consumption is done for accountplus flow? A quick search of StaticGas on authenticators suggests they are all 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some gas here. we only support secp256k1 so it's a static value.

customante.NewSigVerificationDecorator(
options.AccountKeeper,
options.SignModeHandler,
),
),
),
consumeTxSizeGas: ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper),
Expand All @@ -142,8 +148,6 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
options.FeegrantKeeper,
options.TxFeeChecker,
),
setPubKey: ante.NewSetPubKeyDecorator(options.AccountKeeper),
sigGasConsume: ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer),
clobRateLimit: clobante.NewRateLimitDecorator(options.ClobKeeper),
clob: clobante.NewClobDecorator(options.ClobKeeper),
marketUpdates: customante.NewValidateMarketUpdateDecorator(
Expand Down Expand Up @@ -175,8 +179,6 @@ type lockingAnteHandler struct {
sigVerification accountplusante.CircuitBreakerDecorator
consumeTxSizeGas ante.ConsumeTxSizeGasDecorator
deductFee ante.DeductFeeDecorator
setPubKey ante.SetPubKeyDecorator
sigGasConsume ante.SigGasConsumeDecorator
clobRateLimit clobante.ClobRateLimitDecorator
clob clobante.ClobDecorator
marketUpdates customante.ValidateMarketUpdateDecorator
Expand Down Expand Up @@ -252,15 +254,9 @@ func (h *lockingAnteHandler) clobAnteHandle(ctx sdk.Context, tx sdk.Tx, simulate
if ctx, err = h.consumeTxSizeGas.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
if ctx, err = h.setPubKey.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
if ctx, err = h.validateSigCount.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
if ctx, err = h.sigGasConsume.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
if ctx, err = h.replayProtection.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
Expand Down Expand Up @@ -422,15 +418,9 @@ func (h *lockingAnteHandler) otherMsgAnteHandle(ctx sdk.Context, tx sdk.Tx, simu
if ctx, err = h.deductFee.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
if ctx, err = h.setPubKey.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
if ctx, err = h.validateSigCount.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
if ctx, err = h.sigGasConsume.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
if ctx, err = h.replayProtection.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
Expand Down
44 changes: 22 additions & 22 deletions protocol/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -1112,6 +1112,28 @@ func New(
app.SubaccountsKeeper,
)

// Initialize authenticators
app.AuthenticatorManager = authenticator.NewAuthenticatorManager()
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],
app.AuthenticatorManager,
[]string{
lib.GovModuleAddress.String(),
},
)
accountplusModule := accountplusmodule.NewAppModule(appCodec, app.AccountPlusKeeper)

clobFlags := clobflags.GetClobFlagValuesFromOptions(appOpts)
logger.Info("Parsed CLOB flags", "Flags", clobFlags)

Expand Down Expand Up @@ -1231,28 +1253,6 @@ func New(
app.VaultKeeper,
)

// Initialize authenticators
app.AuthenticatorManager = authenticator.NewAuthenticatorManager()
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],
app.AuthenticatorManager,
[]string{
lib.GovModuleAddress.String(),
},
)
accountplusModule := accountplusmodule.NewAppModule(appCodec, app.AccountPlusKeeper)

/**** Module Options ****/

// NOTE: we may consider parsing `appOpts` inside module constructors. For the moment
Expand Down
12 changes: 12 additions & 0 deletions protocol/testutil/constants/stateful_orders.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,18 @@ var (
Subticks: 30,
GoodTilOneof: &clobtypes.Order_GoodTilBlockTime{GoodTilBlockTime: 10},
}
LongTermOrder_Bob_Num0_Id0_Clob1_Buy25_Price30_GTBT10 = clobtypes.Order{
OrderId: clobtypes.OrderId{
SubaccountId: Bob_Num0,
ClientId: 0,
OrderFlags: clobtypes.OrderIdFlags_LongTerm,
ClobPairId: 1,
},
Side: clobtypes.Order_SIDE_BUY,
Quantums: 25,
Subticks: 30,
GoodTilOneof: &clobtypes.Order_GoodTilBlockTime{GoodTilBlockTime: 10},
}
LongTermOrder_Bob_Num0_Id0_Clob0_Buy35_Price30_GTBT11 = clobtypes.Order{
OrderId: clobtypes.OrderId{
SubaccountId: Bob_Num0,
Expand Down
12 changes: 6 additions & 6 deletions protocol/x/accountplus/ante/circuit_breaker.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ import (
// the existence of `TxExtension`.
type CircuitBreakerDecorator struct {
cdc codec.BinaryCodec
authenticatorAnteHandlerFlow sdk.AnteDecorator
originalAnteHandlerFlow sdk.AnteDecorator
authenticatorAnteHandlerFlow sdk.AnteHandler
originalAnteHandlerFlow sdk.AnteHandler
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: Address Remaining sdk.AnteDecorator Usages

The recent changes in circuit_breaker.go simplify the interface by transitioning from sdk.AnteDecorator to sdk.AnteHandler. However, the following files still utilize sdk.AnteDecorator:

  • protocol/x/clob/ante/rate_limit.go
  • protocol/x/clob/ante/clob.go
  • protocol/x/accountplus/ante/circuit_breaker_test.go
  • protocol/mocks/AnteDecorator.go
  • protocol/app/ante.go
  • Multiple test files within protocol/app/ante/

To ensure full consistency and prevent potential integration issues, please update these usages to sdk.AnteHandler where appropriate.

🔗 Analysis chain

LGTM! Verify impact on codebase.

The change from sdk.AnteDecorator to sdk.AnteHandler for both authenticatorAnteHandlerFlow and originalAnteHandlerFlow fields simplifies the interface and aligns with the modifications in the rest of the file. This is a good improvement in terms of code clarity and consistency.

To ensure this change doesn't introduce any issues, please run the following script to check for any remaining usages of sdk.AnteDecorator in the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for remaining usages of sdk.AnteDecorator

# Test: Search for sdk.AnteDecorator usages
rg --type go 'sdk\.AnteDecorator'

Length of output: 10145

}

// NewCircuitBreakerDecorator creates a new instance of CircuitBreakerDecorator with the provided parameters.
func NewCircuitBreakerDecorator(
cdc codec.BinaryCodec,
auth sdk.AnteDecorator,
classic sdk.AnteDecorator,
auth sdk.AnteHandler,
jayy04 marked this conversation as resolved.
Show resolved Hide resolved
classic sdk.AnteHandler,
) CircuitBreakerDecorator {
return CircuitBreakerDecorator{
cdc: cdc,
Expand All @@ -44,9 +44,9 @@ func (ad CircuitBreakerDecorator) AnteHandle(
// Check that the authenticator flow is active
if specified, _ := lib.HasSelectedAuthenticatorTxExtensionSpecified(tx, ad.cdc); specified {
// Return and call the AnteHandle function on all the authenticator decorators.
return ad.authenticatorAnteHandlerFlow.AnteHandle(ctx, tx, simulate, next)
return ad.authenticatorAnteHandlerFlow(ctx, tx, simulate)
jayy04 marked this conversation as resolved.
Show resolved Hide resolved
}

// Return and call the AnteHandle function on all the original decorators.
return ad.originalAnteHandlerFlow.AnteHandle(ctx, tx, simulate, next)
return ad.originalAnteHandlerFlow(ctx, tx, simulate)
}
4 changes: 2 additions & 2 deletions protocol/x/accountplus/ante/circuit_breaker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ func (s *AuthenticatorCircuitBreakerAnteSuite) TestCircuitBreakerAnte() {
// Create a CircuitBreaker AnteDecorator
cbd := ante.NewCircuitBreakerDecorator(
s.tApp.App.AppCodec(),
mockTestAuthenticator,
mockTestClassic,
sdk.ChainAnteDecorators(mockTestAuthenticator),
sdk.ChainAnteDecorators(mockTestClassic),
)
anteHandler := sdk.ChainAnteDecorators(cbd)

Expand Down
6 changes: 3 additions & 3 deletions protocol/x/affiliates/e2e/register_affiliate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@ import (
)

func TestRegisterAffiliateInvalidSigner(t *testing.T) {
tApp := testapp.NewTestAppBuilder(t).Build()
ctx := tApp.InitChain()

testCases := []struct {
name string
referee string
Expand Down Expand Up @@ -45,6 +42,9 @@ func TestRegisterAffiliateInvalidSigner(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
tApp := testapp.NewTestAppBuilder(t).Build()
ctx := tApp.InitChain()

msgRegisterAffiliate := types.MsgRegisterAffiliate{
Referee: tc.referee,
Affiliate: tc.affiliate,
Expand Down
Loading
Loading