Skip to content
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

[textual]: Allow offline signing #15864

Closed
facundomedica opened this issue Apr 17, 2023 · 8 comments
Closed

[textual]: Allow offline signing #15864

facundomedica opened this issue Apr 17, 2023 · 8 comments
Assignees
Labels

Comments

@facundomedica
Copy link
Member

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
Copy link
Member

tac0turtle commented Apr 18, 2023

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

@tac0turtle
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

raynaudoe commented Dec 11, 2023

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:

func initClientConfig() (string, interface{}) {
type GasConfig struct {
GasAdjustment float64 `mapstructure:"gas-adjustment"`
}
type CustomClientConfig struct {
clientconfig.Config `mapstructure:",squash"`
GasConfig GasConfig `mapstructure:"gas"`
}

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
Copy link
Member

julienrbrt commented Dec 11, 2023

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.

@educlerici-zondax educlerici-zondax moved this from 📋 Backlog to 👀 Waiting / In review in Cosmos-SDK Apr 12, 2024
@julienrbrt
Copy link
Member

What's the status on this?
I don't see any linked PR, but status says 'Waiting / In review'?

cc @raynaudoe

@raynaudoe
Copy link
Contributor

What's the status on this? I don't see any linked PR, but status says 'Waiting / In review'?

cc @raynaudoe

There was no consensus on which data should be available for offline signing besides denoms.
AFAIK this issue remains on hold waiting for the signmode team to agree upon this.

@julienrbrt julienrbrt moved this from 👀 Waiting / In review to 📋 Backlog in Cosmos-SDK Sep 9, 2024
@tac0turtle
Copy link
Member

closing as wont fix

@tac0turtle tac0turtle closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2024
@github-project-automation github-project-automation bot moved this from 📋 Backlog to 🥳 Done in Cosmos-SDK Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🥳 Done
Development

No branches or pull requests

5 participants