feat: custom get signers #16340

kocubinski · 2023-05-30T19:41:43Z

Description

Introduces an API function in x/tx/signing DefineCustomGetSigners which can be called to register arbitrary signer fetching logic. Example registration with depinject via an invoker is shown in integration tests.

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)

…6112-custom-signers

…osmos-sdk into kocubinski/16112-custom-signers

aaronc · 2023-06-01T16:47:10Z

x/tx/signing/context.go

+// NOTE: if a custom signers function is defined, the message type used to
+// define this function MUST be the concrete type passed to GetSigners,
+// otherwise a runtime type error will occur.
+func DefineCustomGetSigners[T proto.Message](ctx *Context, getSigners func(T) ([][]byte, error)) {


I wonder if this should be done when constructing a context by setting options. We shouldn't really allow this after initialization

I explored how to do this with depinject, with CustomGetSignersFunc as a ManyPerContainerType, but couldn't come up with anything too good. Maybe we can change the API for DefineCustomGetSigners. I think we'd need to drop the Generic and possibly pass the type name in.

Yeah we can refactor the API. If that sounds like the right direction, let's first figure out how this works with the signing context options struct and then we can figure out depinject

aaronc · 2023-06-01T16:47:27Z

runtime/module.go

@@ -113,12 +114,6 @@ func ProvideApp() (
 		return nil, nil, nil, nil, nil, nil, nil, nil, nil, err
 	}

-	// validate the signing context to make sure that messages are properly configured
-	// with cosmos.msg.v1.signer
-	if err := interfaceRegistry.SigningContext().Validate(); err != nil {


Why no validation?

With current app wiring and depinject, I couldn't find anyway to call Validate after an invoker is called. If it's called before the app will error during start up.

Interesting... can we call validate in an invoker?

Yes but DefineCustomGetSigners is also called in an invoker and order matters, right?

julienrbrt

utACK, just one question

julienrbrt · 2023-06-03T06:30:52Z

x/tx/signing/context_test.go

+	// without a custom signer we should get an error
+	require.ErrorContains(t, err, "use DefineCustomGetSigners to specify")
+
+	// create a new context with a custom signer


Just curious, can you overwrite the proto annotation by defining a custom get signer?
Can we have a test with the expected result? (right now I am not sure what is the expected/wanted behavior)

That's a good point. I think the annotation would override the custom signer but I'll definitely make a test for it.

julienrbrt · 2023-06-03T06:32:54Z

tests/integration/tx/context_test.go

+	simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
+)
+
+func ProvideCustomGetSigners() signing.CustomGetSigner {


nice, it will be useful if this could be displayed in the docs ref: #16294

aaronc

Does this work with gogo messages? Is that important?

Also I think we shouldn't allow a custom get signers if the signer proto annotation exists.

I'm not seeing any place that we're doing validation of the signing context

aaronc · 2023-06-05T14:08:31Z

runtime/module.go

+		},
+	}
+	for _, signer := range customGetSigners {
+		signingOptions.DefineCustomGetSigners(signer.MsgType, signer.Fn)


This code does nothing I'm pretty sure. The receiver is a struct not a pointer

This code does something, registers a custom signer fn. If you'd like to prove to this yourself, comment it out and run TestDefineCustomGetSigners in the x/tx integration tests.

Because map is a pointer type?

Yea. I'm doing an iteration and copy of customGetSignerFuncs in in construction of signing.Context so that changing options after construction won't alter context.

kocubinski · 2023-06-05T14:36:32Z

Does this work with gogo messages? Is that important?

No it doesn't, only pulsar messages. I'll explore gogo support a bit more.

Also I think we shouldn't allow a custom get signers if the signer proto annotation exists.

Right now I think the custom signer will be (silently) ignored in favor of the proto annotation. Do you think we should error though? Or just warn.

I'm not seeing any place that we're doing validation of the signing context

cosmos-sdk/runtime/module.go

Line 182 in 9e7a2bb

err = interfaceRegistry.SigningContext().Validate()

…6112-custom-signers

aaronc · 2023-06-05T14:48:53Z

Does this work with gogo messages? Is that important?

No it doesn't, only pulsar messages. I'll explore gogo support a bit more.

One approach could be to move the Any unpacking up to the signing context and then allow specifying the message type when registering custom get signers.

Also I think we shouldn't allow a custom get signers if the signer proto annotation exists.

Right now I think the custom signer will be (silently) ignored in favor of the proto annotation. Do you think we should error though? Or just warn.

I think we should expect an annotation or custom signer but not both

…6112-custom-signers

kocubinski · 2023-06-05T16:22:52Z

One approach could be to move the Any unpacking up to the signing context and then allow specifying the message type when registering custom get signers.

I'm leaning towards just not supporting gogo here. From what I can tell bera generating pulsar types so this shouldn't be a issue.
https://github.com/berachain/polaris/tree/main/cosmos/api/polaris

…6112-custom-signers

x/tx/signing/context.go

+	for k := range options.CustomGetSigners {
+		customGetSignerFuncs[k] = options.CustomGetSigners[k]
+	}


aaronc · 2023-06-05T19:29:21Z

One approach could be to move the Any unpacking up to the signing context and then allow specifying the message type when registering custom get signers.

I'm leaning towards just not supporting gogo here. From what I can tell bera generating pulsar types so this shouldn't be a issue.

https://github.com/berachain/polaris/tree/main/cosmos/api/polaris

Great!

aaronc · 2023-06-05T21:51:32Z

codec/types/interface_registry.go

-	// ValidatorAddressCodec is the validator address codec to use for the registry. It is required.
-	ValidatorAddressCodec address.Codec
+	// SigningOptions are the signing options to use for the registry.
+	SigningOptions signing.Options


Still we just require a signing context to be passed in? Seems like signing options already takes proto files already so there's some redundancy here

We could but signing.Options permits an nil ProtoFileResolver while interface registry does not. What do you think we ought to do?

Maybe just enforce the check at the interface registry level, or allow a nil resolver and default to hybrid resolver. But duplicated fields in both options structures might be confusing right?

aaronc · 2023-06-05T21:52:41Z

runtime/module.go

+		},
+	}
+	for _, signer := range customGetSigners {
+		signingOptions.DefineCustomGetSigners(signer.MsgType, signer.Fn)


Because map is a pointer type?

tac0turtle · 2023-07-16T19:13:56Z

@kocubinski could i pick your brain on this feature this week, trying to document it but feels a bit odd in how we do it

kocubinski · 2023-07-16T20:27:01Z

@kocubinski could i pick your brain on this feature this week, trying to document it but feels a bit odd in how we do it

Definitely

aaronc and others added 4 commits May 16, 2023 10:02

feat: get signers extension interface

b20a1df

WIP on defining custom get signers

badd826

add DefineCustomGetSigners

00eefcf

make vulncheck fix

d8290fe

github-actions bot added the C:x/tx label May 30, 2023

kocubinski added 5 commits May 30, 2023 16:31

add a custom signers test

7a6bc71

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

a275687

…6112-custom-signers

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

eefa8ae

…6112-custom-signers

remove replace

ccdb3c0

still need replace for new annotation

bbfe4aa

kocubinski mentioned this pull request May 31, 2023

Add optional extension interface to manually specify GetSigners #16112

Closed

no custom signers annotation

d8022de

kocubinski force-pushed the kocubinski/16112-custom-signers branch from 91a9688 to d8022de Compare May 31, 2023 20:51

kocubinski added 6 commits May 31, 2023 15:53

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

a44534a

…6112-custom-signers

go mod tidy

a1db0df

rm unused import

3cac445

specify CustomGetSignersFuncs as inputs

c088db2

fix x/tx test

ee6fa69

need x/tx replace in x/nft/go.mod

ea17470

github-actions bot added the C:x/nft label May 31, 2023

kocubinski added 3 commits June 1, 2023 09:58

works, but not how I want it to

5dce52b

simplify and use invoker

49a9013

remove signingContext validation from ProvideApp

2f4772e

kocubinski marked this pull request as ready for review June 1, 2023 16:06

kocubinski requested a review from a team as a code owner June 1, 2023 16:06

Merge branch 'main' into kocubinski/16112-custom-signers

c6af01c

This comment has been minimized.

Sign in to view

kocubinski added 3 commits June 1, 2023 11:11

changelog

b01d359

Merge branch 'kocubinski/16112-custom-signers' of github.com:cosmos/c…

49d6ccd

…osmos-sdk into kocubinski/16112-custom-signers

lint fixg

66aedff

aaronc reviewed Jun 1, 2023

View reviewed changes

seems better

9e7a2bb

github-actions bot added the C:Rosetta Issues and PR related to Rosetta label Jun 1, 2023

kocubinski added 4 commits June 2, 2023 10:17

add comments

de3816c

refactor moving files

eb72031

adjust depinject test to fail during startup

9138f2c

fix comment

410935f

julienrbrt approved these changes Jun 3, 2023

View reviewed changes

tac0turtle mentioned this pull request Jun 5, 2023

feat: custom get signers #16184

Closed

19 tasks

aaronc reviewed Jun 5, 2023

View reviewed changes

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

bdbace3

…6112-custom-signers

kocubinski added 4 commits June 5, 2023 10:15

disallow custom signer with proto annotation in Validate

0d6f08f

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

6955df2

…6112-custom-signers

fix validate logic

5445754

linting fix

e3dd1c3

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

3947ced

…6112-custom-signers

github-advanced-security bot found potential problems Jun 5, 2023

View reviewed changes

x/tx/signing/context.go

Comment on lines +90 to +92

for k := range options.CustomGetSigners {

customGetSignerFuncs[k] = options.CustomGetSigners[k]

}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism

aaronc approved these changes Jun 5, 2023

View reviewed changes

kocubinski added this pull request to the merge queue Jun 6, 2023

Merged via the queue into main with commit bf6edae Jun 6, 2023

kocubinski deleted the kocubinski/16112-custom-signers branch June 6, 2023 14:22

coderabbitai bot mentioned this pull request Apr 22, 2024

chore: sync main changelogs with v0.47.11, v0.50.6, modules and x/tx #20125

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: custom get signers #16340

feat: custom get signers #16340

kocubinski commented May 30, 2023 •

edited

Loading

This comment has been minimized.

aaronc Jun 1, 2023

kocubinski Jun 1, 2023

aaronc Jun 1, 2023

aaronc Jun 1, 2023

kocubinski Jun 1, 2023 •

edited

Loading

aaronc Jun 1, 2023

kocubinski Jun 1, 2023

julienrbrt left a comment

julienrbrt Jun 3, 2023 •

edited

Loading

kocubinski Jun 5, 2023

julienrbrt Jun 3, 2023

aaronc left a comment

aaronc Jun 5, 2023

kocubinski Jun 5, 2023

aaronc Jun 5, 2023

kocubinski Jun 6, 2023

kocubinski commented Jun 5, 2023

aaronc commented Jun 5, 2023

kocubinski commented Jun 5, 2023

aaronc commented Jun 5, 2023

aaronc Jun 5, 2023

kocubinski Jun 6, 2023

aaronc Jun 6, 2023

aaronc Jun 5, 2023

tac0turtle commented Jul 16, 2023

kocubinski commented Jul 16, 2023

feat: custom get signers #16340

feat: custom get signers #16340

Conversation

kocubinski commented May 30, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

This comment has been minimized.

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

kocubinski Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt left a comment

Choose a reason for hiding this comment

julienrbrt Jun 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc left a 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

kocubinski commented Jun 5, 2023

aaronc commented Jun 5, 2023

kocubinski commented Jun 5, 2023

aaronc commented Jun 5, 2023

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

tac0turtle commented Jul 16, 2023

kocubinski commented Jul 16, 2023

kocubinski commented May 30, 2023 •

edited

Loading

kocubinski Jun 1, 2023 •

edited

Loading

julienrbrt Jun 3, 2023 •

edited

Loading