refactor: middleware refactor to change tx.Handler interface #10527

atheeshp · 2021-11-12T07:53:29Z

Description

This PR makes the following big changes:

1. Change the tx.Handler interface

-	CheckTx(ctx context.Context, tx sdk.Tx, req abci.RequestCheckTx) (abci.ResponseCheckTx, error)
+	CheckTx(ctx context.Context, req tx.Request, checkReq tx.RequestCheckTx) (tx.Response, tx.ResponseCheckTx, error)
// same for Deliver and Simulate

where:

type Response struct {
	GasWanted uint64
	GasUsed   uint64
	// MsgResponses is an array containing each Msg service handler's response
	// type, packed in an Any. This will get proto-serialized into the `Data` field
	// in the ABCI Check/DeliverTx responses.
	MsgResponses []*codectypes.Any
	Log          string
	Events       []abci.Event
}

2. Change what gets passed into the ABCI Check/DeliverTx `Data` field

Before, we were passing the concatenation of MsgResponse bytes into the Data. Now we are passing the proto-serialiazation of this struct:

message TxMsgData {
  repeated google.protobuf.Any msg_responses = 2;
}

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...

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.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…ctor

types/tx/middleware.go

amaury1093 · 2021-11-16T20:59:05Z

proto/cosmos/base/abci/v1beta1/abci.proto

+  // data field is deprecated and not populated.
+  repeated MsgData data = 1 [deprecated = true];
+
+  // msg_responses contains the Msg handler responses type packed in Anys.
+  //
+  // Since: cosmos-sdk 0.45
+  repeated google.protobuf.Any msg_responses = 2;


cc @fdymylja, here's a proposal for solving #10496

what about:

Suggested change

// data field is deprecated and not populated.

repeated MsgData data = 1 [deprecated = true];

// msg_responses contains the Msg handler responses type packed in Anys.

//

// Since: cosmos-sdk 0.45

repeated google.protobuf.Any msg_responses = 2;

// data field is deprecated and not populated.

repeated MsgData data = 1 [deprecated = true];

// msg_responses contains the Msg handler responses type packed in Anys.

//

// Since: cosmos-sdk 0.45

repeated bytes msg_responses = 2;

The bytes would be proto.Marshal(MsgXResponse), i.e. the Any.Value.

Pros: less space on disk
Cons: not straight-forward, clients need to know/learn about this

…ctor

baseapp/abci.go

types/tx/middleware.go

x/auth/middleware/tips.go

baseapp/test_helpers.go

…w-refactor

…ctor

…w-refactor

x/auth/tx/service.go

…nto ap/mw-refactor

…w-refactor

amaury1093 · 2021-12-01T09:06:27Z

baseapp/baseapp_test.go

+			return nil, err
+		}
+
+		res.MsgResponses = []*codectypes.Any{any}


Doing all this msgCounter stuff here in baseapp_test is a bit hacky IMO. Let's try to refactor that out in #10282

amaury1093 · 2021-12-01T09:07:33Z

types/msgservice/msg_service.go

-
-			return nil
-		}, noopInterceptor)
+	fdBytesUnzipped := unzip(proto.FileDescriptor(sd.Metadata.(string)))


thanks Frojdi, this works 👍 (ref: #10646)

fdymylja · 2021-12-01T13:36:31Z

types/tx/msgs.go

+// MsgResponse is the interface all Msg server handlers' response types need to
+// implement. It's the interface that's representing all Msg responses packed
+// in Anys.
+type MsgResponse interface{}


Why is MsgResponse in tx package, whilst Msg is in sdk package. Maybe both fit inside the tx package? We could move sdk.Msg to tx and create a type alias in sdk/types to keep backwards compat

We could move sdk.Msg to tx and create a type alias in sdk/types to keep backwards compat

Actually, this creates a sdk & tx dep cycle. would you be okay if I created an issue to track this? Moreover, this PR is getting big, would like to scope it to middleware stuff.

oh i see.. but yeah agreed on keeping the scope small

fdymylja · 2021-12-01T13:38:19Z

types/tx/middleware.go

+
+// Request is the tx request type used in middlewares.
+type Request struct {
+	Tx      sdk.Tx


Will this be replaced by the concrete type?

Yes! #10347

i-norden · 2021-12-01T21:33:49Z

x/auth/middleware/fee.go

 	sdkCtx := sdk.UnwrapSDKContext(ctx)
-	feeTx, ok := tx.(sdk.FeeTx)
+	feeTx, ok := sdkTx.(sdk.FeeTx)


Just a thought, changing the arg to this method to sdk.FeeTx would require moving the type assertion into all the calling methods (CheckTx, DeliverTx, SimulateTx) which would be more lines of code but it would provide better typing for this method since sdk.FeeTx is the only accepted type.

Ignore this, checkDeductFee isn't a public method so the extra typing isn't particularly helpful.

i-norden · 2021-12-01T21:36:54Z

x/auth/middleware/fee.go

@@ -4,8 +4,6 @@ import (
 	"context"
 	"fmt"

-	abci "github.com/tendermint/tendermint/abci/types"


It's really nice how this removes external deps on tendermint pkgs from so many files!

## Description Update ADR after we iterated on the code itself: #10527  --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] 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.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

…10527)  ## Description Closes: cosmos#10484 This PR makes the following big changes: ### 1. Change the tx.Handler interface ```diff - CheckTx(ctx context.Context, tx sdk.Tx, req abci.RequestCheckTx) (abci.ResponseCheckTx, error) + CheckTx(ctx context.Context, req tx.Request, checkReq tx.RequestCheckTx) (tx.Response, tx.ResponseCheckTx, error) // same for Deliver and Simulate ``` where: ```go type Response struct { GasWanted uint64 GasUsed uint64 // MsgResponses is an array containing each Msg service handler's response // type, packed in an Any. This will get proto-serialized into the `Data` field // in the ABCI Check/DeliverTx responses. MsgResponses []*codectypes.Any Log string Events []abci.Event } ``` ### 2. Change what gets passed into the ABCI Check/DeliverTx `Data` field Before, we were passing the concatenation of MsgResponse bytes into the `Data`. Now we are passing the proto-serialiazation of this struct: ```proto message TxMsgData { repeated google.protobuf.Any msg_responses = 2; } ```  --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] 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.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

## Description Update ADR after we iterated on the code itself: cosmos#10527  --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] 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.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

WIP: middleware refactor

aa99314

github-actions bot added the C:x/auth label Nov 12, 2021

atheeshp and others added 3 commits November 15, 2021 17:54

refactor tx.Request

58c09f2

Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/mw-refa…

fd5f84a

…ctor

Add MsgResponses any in sdk.Result

f766aef

amaury1093 reviewed Nov 15, 2021

View reviewed changes

types/tx/middleware.go Outdated Show resolved Hide resolved

amaury1093 mentioned this pull request Nov 16, 2021

feat: Add Tips middleware #10208

Merged

23 tasks

amaury1093 reviewed Nov 16, 2021

View reviewed changes

atheeshp and others added 7 commits November 17, 2021 17:18

add helper functions in abci

c5e516b

Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/mw-refa…

43cda59

…ctor

refactor tips

306c1fc

Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/mw-refa…

f1afaab

…ctor

review changes

caf2e39

Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/mw-refa…

26285bf

…ctor

Fix mock tests

81f013a

amaury1093 reviewed Nov 25, 2021

View reviewed changes

amaury1093 and others added 14 commits November 25, 2021 18:35

Update baseapp/abci.go

b2dbf10

Update baseapp/abci.go

d685963

Update types/tx/middleware.go

043c3d4

Update types/tx/middleware.go

23ef037

tx.Response to abci conversion

fded41a

refactor makeABCIData

dfeac86

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into ap/m…

7d3f868

…w-refactor

Add comments

b75795e

Fix build

fe7dbbc

fix build error

51861fb

fix tests

9783965

Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/mw-refa…

ed08f40

…ctor

fix test

4c2320a

fix tests

cde4010

amaury1093 assigned i-norden Nov 30, 2021

amaury1093 added 4 commits November 30, 2021 16:34

Use repeated bytes in txMsgData

4e9071f

Fix grpc-gateway test

310fee5

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into ap/m…

f7e12fb

…w-refactor

Make proto-gen

39bee75

amaury1093 reviewed Nov 30, 2021

View reviewed changes

x/auth/tx/service.go Outdated Show resolved Hide resolved

Automagically register MsgResponse

39f0bcc

amaury1093 mentioned this pull request Nov 30, 2021

Automagically register MsgResponse #10646

Merged

19 tasks

atheeshp and others added 8 commits December 1, 2021 13:09

review changes

72b0c30

Merge branch 'master' into ap/mw-refactor

df75f83

Merge branch 'ap/mw-refactor' of ssh://github.com/cosmos/cosmos-sdk i…

edffa1b

…nto ap/mw-refactor

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into ap/m…

473ef1f

…w-refactor

Use froydi's trick

cf51edf

Use Any in TxMsgData

5c93517

Finally remove API breaking change

19b7189

Revert unnecessary stuff

ceb97a4

amaury1093 reviewed Dec 1, 2021

View reviewed changes

fdymylja reviewed Dec 1, 2021

View reviewed changes

amaury1093 mentioned this pull request Dec 1, 2021

Move sdk.Msg and other TX stuff to types/tx #10650

Closed

4 tasks

i-norden approved these changes Dec 1, 2021

View reviewed changes

aaronc approved these changes Dec 1, 2021

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Dec 2, 2021

Merge branch 'master' into ap/mw-refactor

9dfdb35

mergify bot merged commit 5d86db3 into master Dec 2, 2021

mergify bot deleted the ap/mw-refactor branch December 2, 2021 06:54

amaury1093 mentioned this pull request Dec 7, 2021

docs(adr): Update ADR-045 with new middleware interface #10693

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: middleware refactor to change tx.Handler interface #10527

refactor: middleware refactor to change tx.Handler interface #10527

atheeshp commented Nov 12, 2021 •

edited by amaury1093

Loading

amaury1093 Nov 16, 2021

fdymylja Nov 18, 2021

amaury1093 Nov 30, 2021 •

edited

Loading

amaury1093 Dec 1, 2021

amaury1093 Dec 1, 2021

fdymylja Dec 1, 2021

amaury1093 Dec 1, 2021

amaury1093 Dec 1, 2021

amaury1093 Dec 1, 2021

fdymylja Dec 1, 2021

fdymylja Dec 1, 2021

amaury1093 Dec 1, 2021

i-norden Dec 1, 2021

i-norden Dec 1, 2021

i-norden Dec 1, 2021

refactor: middleware refactor to change tx.Handler interface #10527

refactor: middleware refactor to change tx.Handler interface #10527

Conversation

atheeshp commented Nov 12, 2021 • edited by amaury1093 Loading

Description

1. Change the tx.Handler interface

2. Change what gets passed into the ABCI Check/DeliverTx Data field

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Nov 30, 2021 • edited Loading

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

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

atheeshp commented Nov 12, 2021 •

edited by amaury1093

Loading

2. Change what gets passed into the ABCI Check/DeliverTx `Data` field

amaury1093 Nov 30, 2021 •

edited

Loading