cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.08k stars 3.5k forks source link

[textual]: Allow offline signing #15864

Open facundomedica opened 1 year ago

facundomedica commented 1 year ago

Summary

Signmode textual must be able to sign offline.

Problem Definition

Sign mode textual requires a GRPC connection or a bank keeper to query a denom's metadata in order to display it correctly; none of which will/should be available if --offline is being passed.

Proposal

Given that denoms metadata is something that doesn't change often, we can allow the signer to provide the denom metadata when doing an offline signature. This can be done by storing the response of x/bank's DenomsMetadata in a json and then passing that to the CLI through a flag.

tac0turtle commented 1 year ago

why is this required? its not required with the current signing methods

tac0turtle commented 1 year ago

thinking through the use cases of offline signing i can only think multisig but even then youcould use a node. As long as there is a way to sign and not broadcast at the same time i think its chill

raynaudoe commented 7 months ago

What do guys think of doing the following: create a new CoinMetadataQueryFunction in x/auth/tx/config/config.go that receives a DenomsMetadata and returns the metadata properly formatted for the desired denom? Something like:

func NewCachedCoinMetadataQueryFn(m DenomsMetadata) textual.CoinMetadataQueryFn

Then at flags processing time the function can be properly injected

raynaudoe commented 6 months ago

ok, so regarding the toml config to contain DenomsMetadata, I see that I can directly modify the DefaultClientConfigTemplate to include a placeholder for the metadata, alongside with some other changes in config.go. But I also noticed that for example here: https://github.com/cosmos/cosmos-sdk/blob/4b8be25516d1488ae155773f9a73ea049c5ff1cd/simapp/simd/cmd/config.go#L30-L39 the Config struct is being composed to include GasConfig struct which is not present in the DefaultClientConfigTemplate.

I think that modifying the default toml and Config would be a cleaner way to include denoms metadata (maybe backwards compatibilities issues?).... but I'm not sure since the GasConfig inclusion is not done in that way 🤔

julienrbrt commented 6 months ago

but I'm not sure since the GasConfig inclusion is not done in that way

This is done as an example on how chains can extend themselves the client.toml, like they can do for the app.toml. As you can see the logic is in simapp, so it is chain specific.

If we go the toml way, it should be standard for all chains, so it should indeed be added in the default template.