[Spike] feegrants as extension to xaccounts #22252

alpe · 2024-10-14T10:11:47Z

Description

Another option to #22236 that works without composition or a new account type.
The feegrants are a handles as a cross cutting concern to all xaccount types.

As in the other spike, not implemented:

query example
use cached context on queries
legacy data migration
extension config updates
state export/import is done via xaccounts keeper. Not super readable as binary key/value pairs but will work
non conflicting store prefix
tests, doc, ...

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

included the correct type prefix in the PR title, you can find examples of the prefixes below:
confirmed ! in the type prefix if API or client breaking change
targeted the correct branch (see PR Targeting)
provided a link to the relevant issue or specification
reviewed "Files changed" and left comments if necessary
included the necessary unit and integration tests
added a changelog entry to CHANGELOG.md
updated the relevant documentation or specification, including comments for documenting Go code
confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

confirmed the correct type prefix in the PR title
confirmed all author checklist items have been addressed
reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

alpe · 2024-10-14T10:14:03Z

x/accounts/internal/implementation/implementation.go

+	}
+	ext := NewExtensionExecuteAdapter()
+	for _, e := range extensions {
+		if err := ext.RegisterExtension(newDeps(), e); err != nil {


here all the extensions are registered with their proto msg handlers collected

alpe · 2024-10-14T10:15:25Z

x/accounts/internal/implementation/implementation.go

@@ -79,6 +95,10 @@ func newImplementation(schemaBuilder *collections.SchemaBuilder, account Account
 	if err != nil {
 		return Implementation{}, err
 	}
+	executeHandler, err = ext.ExtendHandlers(executeHandler)


The xaccount handlers are decorated by the extension adapter

alpe · 2024-10-14T10:18:37Z

x/accounts/internal/implementation/protoaccount.go

-	}
-
-	router.handlers[reqName] = func(ctx context.Context, executeRequest transaction.Msg) (executeResponse transaction.Msg, err error) {
+	fn := func(ctx context.Context, executeRequest transaction.Msg) (executeResponse transaction.Msg, err error) {


the error is returned in this helper. the registry takes care of the duplicate prevention and internal state

alpe · 2024-10-14T10:19:15Z

x/accounts/keeper.go

@@ -454,4 +456,7 @@ func registerToInterfaceRegistry(ir InterfaceRegistry, accMap map[string]impleme
 			ir.RegisterImplementations(msgInterfaceType, query.RequestSchema.New(), query.ResponseSchema.New())
 		}
 	}
+	for h := range extensions.HandlerSchemas() {


here the protos for the extension are registered

alpe · 2024-10-14T10:20:27Z

x/accounts/internal/implementation/extension.go

+		if len(e.handlers) != 0 {
+			messageName := MessageName(msg)
+			handler, ok := e.handlers[messageName]
+			if ok {


when a matching extension handler exists, it is used. Otherwise, fall though to the account handlers

alpe · 2024-10-14T10:29:12Z

x/accounts/extensions/feegrant/feegrant.go

+	}
+	implementation.RegisterExecuteHandler(reg, f.Init)
+	implementation.RegisterExecuteHandler(reg, f.GrantAllowance)
+	implementation.RegisterExecuteHandler(reg, f.UseGrantedFees)


personal preference: I prefer all the wiring in the constructor. People would look here when doing copy paste for custom types. Methods can easily be missed when not required by an interface

api/cosmos/accounts/extensions/feegrant/v1/feegrant.pulsar.go

+	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
+	anypb "google.golang.org/protobuf/types/known/anypb"
+	io "io"
+	reflect "reflect"


api/cosmos/accounts/extensions/feegrant/v1/feegrant.pulsar.go

+		x.Allowance = value.Message().Interface().(*anypb.Any)
+	default:
+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.extensions.feegrant.v1.Grant"))


api/cosmos/accounts/extensions/feegrant/v1/feegrant.pulsar.go

+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.extensions.feegrant.v1.Grant"))
+		}
+		panic(fmt.Errorf("message cosmos.accounts.extensions.feegrant.v1.Grant does not contain field %s", fd.FullName()))


alpe · 2024-10-15T16:09:47Z

x/accounts/extensions/feegrant/feegrant.go

+}
+
+// MigrateFromLegacy migrate data for the account from x/feegrant module
+func (f *Feegrant) MigrateFromLegacy(ctx context.Context) error {


this method is called right after the account migration

alpe · 2024-10-15T16:11:14Z

x/feegrant/keeper/keeper.go

@@ -61,6 +73,26 @@ func NewKeeper(env appmodule.Environment, cdc codec.BinaryCodec, addrCdc address

 // GrantAllowance creates a new grant
 func (k Keeper) GrantAllowance(ctx context.Context, granter, grantee sdk.AccAddress, feeAllowance feegrant.FeeAllowanceI) error {
+	migrated, err := k.isXAccount(ctx, granter)


This is a big downsite of migrating to xaccounts. Now we have to check for an existing xaccount all the time.

I put the check on top as this was quicker to build but it would probably be more efficient to check state in the module first.
fyi: I also had a "migrated accounts" index in the beginning but accounts can be created in xaccounts without migration, so this did not work

api/cosmos/feegrant/v1beta1/tx.pulsar.go

+		x.Granter = value.Interface().(string)
+	default:
+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.feegrant.v1beta1.MsgMigrateAllowances"))


api/cosmos/feegrant/v1beta1/tx.pulsar.go

+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.feegrant.v1beta1.MsgMigrateAllowances"))
+		}
+		panic(fmt.Errorf("message cosmos.feegrant.v1beta1.MsgMigrateAllowances does not contain field %s", fd.FullName()))


api/cosmos/feegrant/v1beta1/tx.pulsar.go

+		x.Grants = *clv.list
+	default:
+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.feegrant.v1beta1.MsgMigrateAllowancesResponse"))


api/cosmos/feegrant/v1beta1/tx.pulsar.go

+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.feegrant.v1beta1.MsgMigrateAllowancesResponse"))
+		}
+		panic(fmt.Errorf("message cosmos.feegrant.v1beta1.MsgMigrateAllowancesResponse does not contain field %s", fd.FullName()))


julienrbrt · 2024-10-17T07:25:15Z

cc @testinginprod could you check?

Start feegrant as account + extension

cce3142

github-actions bot added C:x/feegrant C:x/accounts labels Oct 14, 2024

alpe commented Oct 14, 2024

View reviewed changes

alpe force-pushed the alex/21747_feegrant_spike2 branch from dbc9908 to f3f1dda Compare October 14, 2024 10:24

alpe commented Oct 14, 2024

View reviewed changes

Refactor to extension to work with all account types

069ed78

alpe force-pushed the alex/21747_feegrant_spike2 branch from f3f1dda to 069ed78 Compare October 14, 2024 10:29

github-advanced-security bot found potential problems Oct 14, 2024

View reviewed changes

alpe added 2 commits October 14, 2024 17:39

Add query example to feegrant extension

3e0265e

Support migration to xaccounts

430893d

github-actions bot added the C:Simulations label Oct 15, 2024

alpe commented Oct 15, 2024

View reviewed changes

github-advanced-security bot found potential problems Oct 15, 2024

View reviewed changes

Minor update

a0c4b64

julienrbrt assigned testinginprod Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spike] feegrants as extension to xaccounts #22252

[Spike] feegrants as extension to xaccounts #22252

alpe commented Oct 14, 2024 •

edited

Loading

alpe Oct 14, 2024

alpe Oct 14, 2024

alpe Oct 14, 2024

alpe Oct 14, 2024

alpe Oct 14, 2024

alpe Oct 14, 2024

alpe Oct 15, 2024

alpe Oct 15, 2024

alpe Oct 17, 2024

julienrbrt commented Oct 17, 2024

[Spike] feegrants as extension to xaccounts #22252

Are you sure you want to change the base?

[Spike] feegrants as extension to xaccounts #22252

Conversation

alpe commented Oct 14, 2024 • edited Loading

Description

Author Checklist

Reviewers Checklist

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

julienrbrt commented Oct 17, 2024

alpe commented Oct 14, 2024 •

edited

Loading