-
Notifications
You must be signed in to change notification settings - Fork 104
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,15 +124,22 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { | |
), | ||
sigVerification: accountplusante.NewCircuitBreakerDecorator( | ||
options.Codec, | ||
accountplusante.NewAuthenticatorDecorator( | ||
options.Codec, | ||
options.AccountplusKeeper, | ||
options.AccountKeeper, | ||
options.SignModeHandler, | ||
sdk.ChainAnteDecorators( | ||
customante.NewEmitPubKeyEventsDecorator(), | ||
accountplusante.NewAuthenticatorDecorator( | ||
options.Codec, | ||
options.AccountplusKeeper, | ||
options.AccountKeeper, | ||
options.SignModeHandler, | ||
), | ||
), | ||
customante.NewSigVerificationDecorator( | ||
options.AccountKeeper, | ||
options.SignModeHandler, | ||
sdk.ChainAnteDecorators( | ||
ante.NewSetPubKeyDecorator(options.AccountKeeper), | ||
ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||
|
@@ -142,8 +149,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( | ||
|
@@ -175,8 +180,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 | ||
|
@@ -252,15 +255,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 | ||
} | ||
|
@@ -422,15 +419,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 | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
package ante | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @teddyding I added a new decorator that looks identical to reference from Osmosis (link) |
||
|
||
import ( | ||
"encoding/base64" | ||
"fmt" | ||
|
||
errorsmod "cosmossdk.io/errors" | ||
|
||
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/cosmos/cosmos-sdk/types/tx/signing" | ||
authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" | ||
) | ||
|
||
// NewEmitPubKeyEventsDecorator emits events for each signer's public key. | ||
// CONTRACT: Tx must implement SigVerifiableTx interface | ||
type EmitPubKeyEventsDecorator struct{} | ||
|
||
func NewEmitPubKeyEventsDecorator() EmitPubKeyEventsDecorator { | ||
return EmitPubKeyEventsDecorator{} | ||
} | ||
|
||
func (eped EmitPubKeyEventsDecorator) AnteHandle( | ||
ctx sdk.Context, | ||
tx sdk.Tx, | ||
simulate bool, | ||
next sdk.AnteHandler, | ||
) (sdk.Context, error) { | ||
sigTx, ok := tx.(authsigning.SigVerifiableTx) | ||
if !ok { | ||
return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid tx type") | ||
} | ||
|
||
signers, err := sigTx.GetSigners() | ||
if err != nil { | ||
return ctx, err | ||
} | ||
|
||
signerStrs := make([]string, len(signers)) | ||
|
||
jayy04 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Also emit the following events, so that txs can be indexed by these | ||
// indices: | ||
// - signature (via `tx.signature='<sig_as_base64>'`), | ||
// - concat(address,"/",sequence) (via `tx.acc_seq='cosmos1abc...def/42'`). | ||
sigs, err := sigTx.GetSignaturesV2() | ||
if err != nil { | ||
return ctx, err | ||
} | ||
|
||
var events sdk.Events | ||
for i, sig := range sigs { | ||
events = append(events, sdk.NewEvent(sdk.EventTypeTx, | ||
sdk.NewAttribute(sdk.AttributeKeyAccountSequence, fmt.Sprintf("%s/%d", signerStrs[i], sig.Sequence)), | ||
)) | ||
|
||
sigBzs, err := signatureDataToBz(sig.Data) | ||
if err != nil { | ||
return ctx, err | ||
} | ||
for _, sigBz := range sigBzs { | ||
events = append(events, sdk.NewEvent(sdk.EventTypeTx, | ||
sdk.NewAttribute(sdk.AttributeKeySignature, base64.StdEncoding.EncodeToString(sigBz)), | ||
)) | ||
} | ||
} | ||
|
||
ctx.EventManager().EmitEvents(events) | ||
|
||
return next(ctx, tx, simulate) | ||
} | ||
|
||
// signatureDataToBz converts a SignatureData into raw bytes signature. | ||
// For SingleSignatureData, it returns the signature raw bytes. | ||
// For MultiSignatureData, it returns an array of all individual signatures, | ||
// as well as the aggregated signature. | ||
func signatureDataToBz(data signing.SignatureData) ([][]byte, error) { | ||
if data == nil { | ||
return nil, fmt.Errorf("got empty SignatureData") | ||
} | ||
|
||
switch data := data.(type) { | ||
case *signing.SingleSignatureData: | ||
return [][]byte{data.Signature}, nil | ||
case *signing.MultiSignatureData: | ||
sigs := [][]byte{} | ||
var err error | ||
|
||
for _, d := range data.Signatures { | ||
nestedSigs, err := signatureDataToBz(d) | ||
if err != nil { | ||
return nil, err | ||
} | ||
sigs = append(sigs, nestedSigs...) | ||
} | ||
|
||
multiSignature := cryptotypes.MultiSignature{ | ||
Signatures: sigs, | ||
} | ||
aggregatedSig, err := multiSignature.Marshal() | ||
if err != nil { | ||
return nil, err | ||
} | ||
sigs = append(sigs, aggregatedSig) | ||
|
||
return sigs, nil | ||
default: | ||
return nil, sdkerrors.ErrInvalidType.Wrapf("unexpected signature data type %T", data) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,11 @@ func (sva SignatureVerification) Initialize(config []byte) (types.Authenticator, | |
// Authenticate takes a SignaturesVerificationData struct and validates | ||
// each signer and signature using signature verification | ||
func (sva SignatureVerification) Authenticate(ctx sdk.Context, request types.AuthenticationRequest) error { | ||
// First consume gas for verifying the signature | ||
params := sva.ak.GetParams(ctx) | ||
ctx.GasMeter().ConsumeGas(params.SigVerifyCostSecp256k1, "secp256k1 signature verification") | ||
Comment on lines
+59
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added gas here @teddyding . we currently only support secp256k1 signature verification so charging some static gas here. |
||
|
||
// after gas consumption continue to verify signatures | ||
jayy04 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if request.Simulate || ctx.IsReCheckTx() { | ||
return nil | ||
} | ||
|
There was a problem hiding this comment.
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
andSigGas
under this chain and thus not applicable to smart account flow?There was a problem hiding this comment.
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 somepublic key = nil
errors inSigGas
here because public key is not set. So moved this as well.This is also how osmosis does it (link)