refactor!: use x/tx/signing in client.TxConfig #15822

kocubinski · 2023-04-12T17:54:23Z

Description

This patch changes the type of client.TxConfig.SignModeHandler from x/auth/tx/SignModeHandler to x/tx/SignModeHandler, then fixes all resulting breakage. This deprecates signing in x/auth/tx in favor of the packages in x/tx.

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)

client/tx/tx.go

…x-config-break

kocubinski · 2023-04-19T15:04:27Z

I would prefer to see the merged registry as a parameter rather than embedded as calls to functions, just as a general matter of avoiding global state. But if there are reasons we can't do that, oh well.

I 💯 agree with merged registry as a parameter. When code uses depinject this is doable, but if we're supporting non-depinject initialization we can only do this by introducing further API breakage, specifically NewTxConfig but I'm not sure if that's the best choice.

If we're willing to break NewTxConfig with changed arguments and add a signing.ProtoFileResolver to both sdk.Context and client.Context I think we could potentially remove the global. Maybe this could be a follow on PR?

@tac0turtle @aaronc curious your thoughts on breaking NewTxConfig?

…x-config-break

aaronc · 2023-04-19T20:05:25Z

I would prefer to see the merged registry as a parameter rather than embedded as calls to functions, just as a general matter of avoiding global state. But if there are reasons we can't do that, oh well.

I 💯 agree with merged registry as a parameter. When code uses depinject this is doable, but if we're supporting non-depinject initialization we can only do this by introducing further API breakage, specifically NewTxConfig but I'm not sure if that's the best choice.

If we're willing to break NewTxConfig with changed arguments and add a signing.ProtoFileResolver to both sdk.Context and client.Context I think we could potentially remove the global. Maybe this could be a follow on PR?

@tac0turtle @aaronc curious your thoughts on breaking NewTxConfig?

We don't need to change anything. NewTxConfig already gets ProtoCodecMarshaler which has an InterfaceRegistry which implements signing.ProtoFileResolver

aaronc · 2023-04-19T20:07:58Z

x/auth/tx/config.go

+) client.TxConfig {
+	// protoFiles should perhaps be a parameter to this function, but the choice was made here to not break the
+	// NewTxConfig API.
+	protoFiles := registry.MergedProtoRegistry()


Suggested change

protoFiles := registry.MergedProtoRegistry()

aaronc · 2023-04-19T20:09:00Z

x/auth/tx/config.go

+	// NewTxConfig API.
+	protoFiles := registry.MergedProtoRegistry()
+	typeResolver := protoregistry.GlobalTypes
+	signersContext, err := txsigning.NewGetSignersContext(txsigning.GetSignersOptions{ProtoFiles: protoFiles})


This is unnecessary because of #15873. Just use codec.InterfaceRegistry().SigningContext()

This is a compile error right now. I think it is OK to land this in main first then work on #15873 (which is still failing tests) then refactor?

kocubinski · 2023-04-19T21:15:46Z

We don't need to change anything. NewTxConfig already gets ProtoCodecMarshaler which has an InterfaceRegistry which implements signing.ProtoFileResolver

I hadn't noticed that InterfaceRegistry implements signing.ProtoFileResolver, good point. I think we're just pushing the problem around? I hadn't notice the ref to the global proto.MergedRegistry in the NewInterfaceRegistry either. So this constructor would need to tolerate an API break to have it injected, right?

Personally I'd prefer breaking the API, and maybe NewInterfaceRegistry is the right place.

aaronc · 2023-04-19T21:28:51Z

See how it's handled in #15873. Changing NewInterfaceRegistry will be too painful on tests. We just have an overload with options to deal with this

kocubinski · 2023-04-20T15:03:12Z

I am leaning toward merging and then refactoring out the global proto.MergedRegistry usages after #15873 lands since I'll be working on that anyway.

aaronc · 2023-04-20T15:19:43Z

I am leaning toward merging and then refactoring out the global proto.MergedRegistry usages after #15873 lands since I'll be working on that anyway.

But if you have a Codec you already have the MergedRegistry, no need to wait for #15873 for that part

it is being called in init() lifecycle before all proto files are loaded. the result is much wasted work right now.

julienrbrt · 2023-04-20T16:37:30Z

x/auth/tx/config.go

-	return NewTxConfigWithHandler(protoCodec, makeSignModeHandler(enabledSignModes, textual, customSignModes...))
+//
+// Deprecated: use NewTxConfigWithOptions instead.
+func NewTxConfigWithTextual(protoCodec codec.ProtoCodecMarshaler, _ []signingtypes.SignMode,


This has never been released, is there a reason to deprecate it instead of completely removing it?

No, I'll remove it in a follow up.

julienrbrt · 2023-04-20T16:39:59Z

x/auth/tx/config.go

+				TypeResolver: typeResolver,
+				Encoder:      &aminoJSONEncoder,
+			}
+		case signingtypes.SignMode_SIGN_MODE_TEXTUAL:


I think the panic message should be changed with the right function

julienrbrt · 2023-04-20T16:40:19Z

x/auth/tx/config.go

@@ -27,25 +31,66 @@ type config struct {
 // NOTE: Use NewTxConfigWithHandler to provide a custom signing handler in case the sign mode
 // is not supported by default (eg: SignMode_SIGN_MODE_EIP_191). Use NewTxConfigWithTextual


kocubinski added 4 commits April 12, 2023 09:46

Hard break client.TxConfig

db53c1d

incorporate changes from #15713

0019cb9

Hard fixing

f44fee6

Merge branch 'main' into kocubinski/tx-config-break

49c18df

github-actions bot added C:CLI C:Rosetta Issues and PR related to Rosetta C:x/auth C:x/tx labels Apr 12, 2023

kocubinski added 2 commits April 12, 2023 13:00

go mod tidy

14539c4

fix breakage in codec from latest main

5f2b4de

github-advanced-security bot found potential problems Apr 12, 2023

View reviewed changes

client/tx/tx.go Fixed Show fixed Hide fixed

kocubinski added 4 commits April 13, 2023 10:21

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/t…

02c2340

…x-config-break

remove replace for x/tx

147cbdf

revert codec/ compat for x/tx 0.6.0

55e1d36

x/tx v0.5.1

55acfe3

kocubinski changed the title ~~[WIP] refactor: break client.TxConfig API with x/tx signers~~ refactor: [WIP] break client.TxConfig API with x/tx signers Apr 13, 2023

kocubinski added 9 commits April 13, 2023 10:28

add err handling

bedd4b9

checkpoint, proto gen

21927ef

adjust lots of tests

623bacf

fixed ante tests

43ea1b4

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/t…

e32f007

…x-config-break

merged fix

bff8090

revert (temporarily) x/tx to 0.5.1

0a25910

rm files

3f0a4f2

fix tx_test

d24dbc2

github-actions bot added the C:x/authz label Apr 13, 2023

revert x/tx

2e62758

github-actions bot added the C:x/group label Apr 13, 2023

kocubinski added 2 commits April 14, 2023 10:58

comment

2c9bd78

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/t…

863c869

…x-config-break

kocubinski added 5 commits April 19, 2023 10:31

Add UPGRADING entries

f3211b8

Merge branch 'main' into kocubinski/tx-config-break

147dc83

PR fixes

d1af44c

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/t…

e8398d2

…x-config-break

lint fix

cd4f11f

kocubinski mentioned this pull request Apr 19, 2023

Migrate CLI commands in x/auth/tx to comet txserver #15888

Open

aaronc reviewed Apr 19, 2023

View reviewed changes

kocubinski added 2 commits April 20, 2023 10:34

Refactor NewTxConfig

2bb6750

lazy loading won't work in InterfaceRegistry ctr won't work yet.

3bcaa98

it is being called in init() lifecycle before all proto files are loaded. the result is much wasted work right now.

kocubinski enabled auto-merge April 20, 2023 16:09

kocubinski added this pull request to the merge queue Apr 20, 2023

Merged via the queue into main with commit 86b88b1 Apr 20, 2023

kocubinski deleted the kocubinski/tx-config-break branch April 20, 2023 16:22

julienrbrt reviewed Apr 20, 2023

View reviewed changes

kocubinski mentioned this pull request Apr 24, 2023

refactor(x/auth): v2 adaptable tx instead of double decode #15910

Merged

19 tasks

kocubinski added a commit that referenced this pull request Apr 24, 2023

refactor(x/auth): doc and remove deprecated fn from #15822

2c38358

kocubinski mentioned this pull request Apr 24, 2023

refactor(x/auth): doc and remove deprecated fn from #15822 #15932

Merged

19 tasks

kocubinski added a commit that referenced this pull request Apr 24, 2023

refactor(x/auth): doc and remove deprecated fn from #15822 (#15932)

dde6fcd

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

refactor!: use x/tx/signing in client.TxConfig (cosmos#15822)

5c71080

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: use x/tx/signing in client.TxConfig #15822

refactor!: use x/tx/signing in client.TxConfig #15822

kocubinski commented Apr 12, 2023 •

edited

Loading

kocubinski commented Apr 19, 2023 •

edited

Loading

aaronc commented Apr 19, 2023 •

edited

Loading

aaronc Apr 19, 2023

aaronc Apr 19, 2023

kocubinski Apr 19, 2023

kocubinski commented Apr 19, 2023 •

edited

Loading

aaronc commented Apr 19, 2023

kocubinski commented Apr 20, 2023

aaronc commented Apr 20, 2023

julienrbrt Apr 20, 2023

kocubinski Apr 20, 2023

julienrbrt Apr 20, 2023

julienrbrt Apr 20, 2023

		@@ -27,25 +31,66 @@ type config struct {
		// NOTE: Use NewTxConfigWithHandler to provide a custom signing handler in case the sign mode
		// is not supported by default (eg: SignMode_SIGN_MODE_EIP_191). Use NewTxConfigWithTextual

refactor!: use x/tx/signing in client.TxConfig #15822

refactor!: use x/tx/signing in client.TxConfig #15822

Conversation

kocubinski commented Apr 12, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

kocubinski commented Apr 19, 2023 • edited Loading

aaronc commented Apr 19, 2023 • edited Loading

aaronc Apr 19, 2023

Choose a reason for hiding this comment

aaronc Apr 19, 2023

Choose a reason for hiding this comment

kocubinski Apr 19, 2023

Choose a reason for hiding this comment

kocubinski commented Apr 19, 2023 • edited Loading

aaronc commented Apr 19, 2023

kocubinski commented Apr 20, 2023

aaronc commented Apr 20, 2023

julienrbrt Apr 20, 2023

Choose a reason for hiding this comment

kocubinski Apr 20, 2023

Choose a reason for hiding this comment

julienrbrt Apr 20, 2023

Choose a reason for hiding this comment

julienrbrt Apr 20, 2023

Choose a reason for hiding this comment

kocubinski commented Apr 12, 2023 •

edited

Loading

kocubinski commented Apr 19, 2023 •

edited

Loading

aaronc commented Apr 19, 2023 •

edited

Loading

kocubinski commented Apr 19, 2023 •

edited

Loading