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

[RFC] change(gnokey): use filename as arg in sign cmd #877

Closed
wants to merge 1 commit into from

Conversation

harry-hov
Copy link
Contributor

@harry-hov harry-hov commented Jun 7, 2023

Description

Currently gnokey sign takes keyname/address as arg and filename from flag --txPath. This PR reverses that order.
Now it takes filename as arg and keyname/address from flag --key.

Because:

  • it makes more sense to me this way

  • it would be easier to accommodate multiple filenames in future (if we want to support that).

    Something like:

    gnokey sign multiple-unsigned.tx unsigned1.tx unsigned2.tx ... --key=foo > combined-signed.tx

    (see Support generating multi msg transaction with gnokey #845)

I would like to have your opinion on this. Please consider this PR as a place to discuss about this.


Contributors Checklist

  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

Maintainers Checklist

  • Checked that the author followed the guidelines in CONTRIBUTING.md
  • Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
  • Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

@harry-hov harry-hov requested a review from a team June 7, 2023 19:42
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jun 7, 2023
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm against introducing such a change into the gnokey codebase, for a few reasons:

  • it invalidates practically all tutorials / guides / video workshops we have created, since a critical API is changed
  • if we wanted to support multiple files for the input of gnokey sign, it's trivial to do it with the ff we're using, no need to have these as command arguments
  • you sign multiple things with a single key, and not any other way (multiple keys for multiple things) in a single command run. This is why the key is the argument (single, required)

I would 💯 support an effort to have the key be a flag (or frankly, everything be a flag), instead of having arguments which caused headaches in the past, but this is not the topic of the PR

cc @moul

@thehowl
Copy link
Member

thehowl commented Jul 20, 2023

Key is obviously required, so I don't think it makes sense to have it as a flag. I don't think we'll need multiple keys, so maybe something we could do is to extend the command, maybe making the -txpath flag hidden if it's possible (for compatibility with old tutorials) and make the new syntax gnokey sign [flags] <key-name-or-address> [<file>...].

@harry-hov
Copy link
Contributor Author

harry-hov commented Jul 20, 2023

I very much like the sign API of cosmos-based chains

(binary)d tx sign <file> --from $ADDRESS

So I wanted to discuss the possibility of making it uniform with cosmos or better.

I think it's a good time to discuss BREAKING API changes if it improves gnokey.
(Since I believe we are still early stage, It'll be tough after mass adoption or mainnet)
(Documentation should keep evolving with code changes if it's for better future)

Cosmos also has a very nice implementation of multisign(multiple keys for single tx). Maybe we want to take a look.

Opened this PR to initiate a discussion on this. that's why It's marked as RFC.

Thanks.

(Maybe I'm just used to cosmos sign API :p)

@thehowl
Copy link
Member

thehowl commented Jul 21, 2023

I think it's a good time to discuss BREAKING API changes if it improves gnokey. (Since I believe we are still early stage, It'll be tough after mass adoption or mainnet) (Documentation should keep evolving with code changes if it's for better future)

Cosmos also has a very nice implementation of multisign(multiple keys for single tx). Maybe we want to take a look.

Opened this PR to initiate a discussion on this. that's why It's marked as RFC.

Agreed & understood, just wanted to throw in my 2c.

I don't know enough about multisign; but don't think we necessarily need to do the same as cosmos sdk without good reason.

@harry-hov
Copy link
Contributor Author

I don't know enough about multisign; but don't think we necessarily need to do the same as cosmos sdk without good reason.

I agree with you 💯

But we can always learn from other implementations out there. And implement something which is better and fits our usecase.

@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
@thehowl
Copy link
Member

thehowl commented Jan 18, 2024

So, we discussed this during the review meeting (and together with this, also came to a decision on short flags, #1555).

We can go through with this change, so we make consistent throughout gnokey that the way to pass the key you want to use is through the -key parameter.

Please, @harry-hov, update this PR with master and ensure that this is consistently applied to all gnokey commands :)

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Status: Done
Status: 🌟 Wanted for Launch
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants