-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat: custom get signers #16340
feat: custom get signers #16340
Changes from all commits
b20a1df
badd826
00eefcf
d8290fe
7a6bc71
a275687
eefa8ae
ccdb3c0
bbfe4aa
d8022de
a44534a
a1db0df
3cac445
c088db2
ee6fa69
ea17470
5dce52b
49a9013
2f4772e
c6af01c
b01d359
49d6ccd
66aedff
9e7a2bb
de3816c
eb72031
9138f2c
410935f
bdbace3
0d6f08f
6955df2
5445754
e3dd1c3
3947ced
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 |
---|---|---|
|
@@ -8,6 +8,7 @@ import ( | |
appv1alpha1 "cosmossdk.io/api/cosmos/app/v1alpha1" | ||
"cosmossdk.io/depinject" | ||
"cosmossdk.io/log" | ||
"cosmossdk.io/x/tx/signing" | ||
"github.com/cosmos/gogoproto/proto" | ||
"google.golang.org/protobuf/reflect/protodesc" | ||
"google.golang.org/protobuf/reflect/protoregistry" | ||
|
@@ -19,6 +20,7 @@ import ( | |
"cosmossdk.io/core/header" | ||
"cosmossdk.io/core/store" | ||
storetypes "cosmossdk.io/store/types" | ||
|
||
"github.com/cosmos/cosmos-sdk/codec/address" | ||
|
||
"github.com/cosmos/cosmos-sdk/baseapp" | ||
|
@@ -60,6 +62,7 @@ func init() { | |
appmodule.Register(&runtimev1alpha1.Module{}, | ||
appmodule.Provide( | ||
ProvideApp, | ||
ProvideInterfaceRegistry, | ||
ProvideKVStoreKey, | ||
ProvideTransientStoreKey, | ||
ProvideMemoryStoreKey, | ||
|
@@ -76,8 +79,7 @@ func init() { | |
) | ||
} | ||
|
||
func ProvideApp() ( | ||
codectypes.InterfaceRegistry, | ||
func ProvideApp(interfaceRegistry codectypes.InterfaceRegistry) ( | ||
codec.Codec, | ||
*codec.LegacyAmino, | ||
*AppBuilder, | ||
|
@@ -98,27 +100,6 @@ func ProvideApp() ( | |
_, _ = fmt.Fprintln(os.Stderr, err.Error()) | ||
} | ||
|
||
interfaceRegistry, err := codectypes.NewInterfaceRegistryWithOptions(codectypes.InterfaceRegistryOptions{ | ||
ProtoFiles: proto.HybridResolver, | ||
// using the global prefixes is a temporary solution until we refactor this | ||
// to get the address.Codec's from the container | ||
AddressCodec: address.Bech32Codec{ | ||
Bech32Prefix: sdk.GetConfig().GetBech32AccountAddrPrefix(), | ||
}, | ||
ValidatorAddressCodec: address.Bech32Codec{ | ||
Bech32Prefix: sdk.GetConfig().GetBech32ValidatorAddrPrefix(), | ||
}, | ||
}) | ||
if err != nil { | ||
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 { | ||
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. Why no validation? 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. 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. 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. Interesting... can we call validate in an invoker? 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. Yes but |
||
return nil, nil, nil, nil, nil, nil, nil, nil, nil, err | ||
} | ||
|
||
amino := codec.NewLegacyAmino() | ||
|
||
std.RegisterInterfaces(interfaceRegistry) | ||
|
@@ -136,7 +117,7 @@ func ProvideApp() ( | |
} | ||
appBuilder := &AppBuilder{app} | ||
|
||
return interfaceRegistry, cdc, amino, appBuilder, cdc, msgServiceRouter, appModule{app}, protoFiles, protoTypes, nil | ||
return cdc, amino, appBuilder, cdc, msgServiceRouter, appModule{app}, protoFiles, protoTypes, nil | ||
} | ||
|
||
type AppInputs struct { | ||
|
@@ -176,6 +157,35 @@ func SetupAppBuilder(inputs AppInputs) { | |
} | ||
} | ||
|
||
func ProvideInterfaceRegistry(customGetSigners []signing.CustomGetSigner) (codectypes.InterfaceRegistry, error) { | ||
signingOptions := signing.Options{ | ||
// using the global prefixes is a temporary solution until we refactor this | ||
// to get the address.Codec's from the container | ||
AddressCodec: address.Bech32Codec{ | ||
Bech32Prefix: sdk.GetConfig().GetBech32AccountAddrPrefix(), | ||
}, | ||
ValidatorAddressCodec: address.Bech32Codec{ | ||
Bech32Prefix: sdk.GetConfig().GetBech32ValidatorAddrPrefix(), | ||
}, | ||
} | ||
for _, signer := range customGetSigners { | ||
signingOptions.DefineCustomGetSigners(signer.MsgType, signer.Fn) | ||
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. This code does nothing I'm pretty sure. The receiver is a struct not a pointer 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. This code does something, registers a custom signer fn. If you'd like to prove to this yourself, comment it out and run 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. Because map is a pointer type? 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. 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. |
||
} | ||
|
||
interfaceRegistry, err := codectypes.NewInterfaceRegistryWithOptions(codectypes.InterfaceRegistryOptions{ | ||
ProtoFiles: proto.HybridResolver, | ||
SigningOptions: signingOptions, | ||
}) | ||
if err != nil { | ||
return nil, err | ||
} | ||
err = interfaceRegistry.SigningContext().Validate() | ||
if err != nil { | ||
return nil, err | ||
} | ||
return interfaceRegistry, nil | ||
} | ||
|
||
func registerStoreKey(wrapper *AppBuilder, key storetypes.StoreKey) { | ||
wrapper.app.storeKeys = append(wrapper.app.storeKeys, key) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
package tx | ||
|
||
import ( | ||
"testing" | ||
|
||
"cosmossdk.io/depinject" | ||
"cosmossdk.io/log" | ||
"cosmossdk.io/x/tx/signing" | ||
"github.com/stretchr/testify/require" | ||
"google.golang.org/protobuf/proto" | ||
|
||
codectypes "github.com/cosmos/cosmos-sdk/codec/types" | ||
"github.com/cosmos/cosmos-sdk/tests/integration/tx/internal/pulsar/testpb" | ||
"github.com/cosmos/cosmos-sdk/testutil/configurator" | ||
simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" | ||
) | ||
|
||
func ProvideCustomGetSigners() signing.CustomGetSigner { | ||
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. nice, it will be useful if this could be displayed in the docs ref: #16294 |
||
return signing.CustomGetSigner{ | ||
MsgType: proto.MessageName(&testpb.TestRepeatedFields{}), | ||
Fn: func(msg proto.Message) ([][]byte, error) { | ||
testMsg := msg.(*testpb.TestRepeatedFields) | ||
// arbitrary logic | ||
signer := testMsg.NullableDontOmitempty[1].Value | ||
return [][]byte{[]byte(signer)}, nil | ||
}, | ||
} | ||
} | ||
|
||
func TestDefineCustomGetSigners(t *testing.T) { | ||
var interfaceRegistry codectypes.InterfaceRegistry | ||
_, err := simtestutil.SetupAtGenesis( | ||
depinject.Configs( | ||
configurator.NewAppConfig( | ||
configurator.ParamsModule(), | ||
configurator.AuthModule(), | ||
configurator.StakingModule(), | ||
configurator.BankModule(), | ||
configurator.ConsensusModule(), | ||
), | ||
depinject.Supply(log.NewNopLogger()), | ||
depinject.Provide(ProvideCustomGetSigners), | ||
), | ||
&interfaceRegistry, | ||
) | ||
require.NoError(t, err) | ||
require.NotNil(t, interfaceRegistry) | ||
|
||
msg := &testpb.TestRepeatedFields{ | ||
NullableDontOmitempty: []*testpb.Streng{ | ||
{Value: "foo"}, | ||
{Value: "bar"}, | ||
}, | ||
} | ||
signers, err := interfaceRegistry.SigningContext().GetSigners(msg) | ||
require.NoError(t, err) | ||
require.Equal(t, [][]byte{[]byte("bar")}, signers) | ||
|
||
// Reset and provider no CustomGetSigners. Consequently, validation will fail and depinject will return an error | ||
_, err = simtestutil.SetupAtGenesis( | ||
depinject.Configs( | ||
configurator.NewAppConfig( | ||
configurator.ParamsModule(), | ||
configurator.AuthModule(), | ||
configurator.StakingModule(), | ||
configurator.BankModule(), | ||
configurator.ConsensusModule(), | ||
), | ||
depinject.Supply(log.NewNopLogger()), | ||
), | ||
&interfaceRegistry, | ||
) | ||
require.ErrorContains(t, err, "use DefineCustomGetSigners") | ||
} |
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.
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
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.
We could but
signing.Options
permits an nilProtoFileResolver
while interface registry does not. What do you think we ought to do?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.
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?