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

feat: simplify the import / export API for the Keybase #3285

Merged
merged 9 commits into from
Dec 18, 2024

Conversation

zivkovicmilos
Copy link
Member

@zivkovicmilos zivkovicmilos commented Dec 6, 2024

Description

This PR greatly simplifies the Keybase interface in regards to key imports / exports, by removing redundant methods.

The goal of the PR is to put the application-level key armor management outside the keybase (not including storage), towards the caller. In fact, this is the reason why the Keybase has a chaotic API for imports / exports.

I've preserved existing gnokey functionality -- this is unchanged.
I will update gnokey imports / exports in a separate PR, to make the flag usage make more sense.

BREAKING CHANGE:

  • Removed 9 Keybase methods, and added 2 new ones (that replace them)

cc @jefft0

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

@zivkovicmilos zivkovicmilos added the breaking change Functionality that contains breaking changes label Dec 6, 2024
@zivkovicmilos zivkovicmilos added this to the ⏭️Next after mainnet milestone Dec 6, 2024
@zivkovicmilos zivkovicmilos self-assigned this Dec 6, 2024
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Dec 6, 2024
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 68.08511% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
tm2/pkg/crypto/keys/client/import.go 71.42% 5 Missing and 1 partial ⚠️
tm2/pkg/crypto/keys/lazy_keybase.go 0.00% 4 Missing ⚠️
tm2/pkg/crypto/keys/client/export.go 75.00% 2 Missing and 1 partial ⚠️
tm2/pkg/crypto/keys/keybase.go 80.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@zivkovicmilos zivkovicmilos marked this pull request as draft December 6, 2024 13:11
@zivkovicmilos zivkovicmilos marked this pull request as ready for review December 6, 2024 13:14
@Kouteki Kouteki added the in focus Core team is prioritizing this work label Dec 6, 2024
tm2/pkg/crypto/keys/types.go Outdated Show resolved Hide resolved
Copy link
Contributor

@aeddi aeddi left a comment

Choose a reason for hiding this comment

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

Two small details, but apart from that, looks good to me 👍

tm2/pkg/crypto/keys/client/export.go Outdated Show resolved Hide resolved
tm2/pkg/crypto/keys/client/import.go Outdated Show resolved Hide resolved
@moul moul requested review from ltzmaxwell and piux2 December 9, 2024 09:44
@moul
Copy link
Member

moul commented Dec 9, 2024

As someone using this package, I prefer this new API. But I would like to summon a few people:

  • @piux2, as I recall he was using this package for hiring purposes and may have insights that I don't.
  • @tbruyelle, for his broader experience in other cosmos chains.

@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 9, 2024

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

No automated checks match this pull request.

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

Copy link
Contributor

@tbruyelle tbruyelle left a comment

Choose a reason for hiding this comment

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

The SDK equivalent Keyring struct exposes a similar API to the actual Keybase of this repo (see 1 2 3) (before this PR), without the method that returns the unencrypted private key (namely ExportPrivateKeyObject()). This method is not part of the public interface, so it's a bit harder to call (example from the CLI export command: 4).

However, I see no reason why the API couldn't be simplified. Methods like Export/ImportPubkey() are not used anywhere in the SDK, yet they have been around for years.

Now about the "obfuscation" of unsafe ExportPrivateKeyObject(), the current change makes a clear difference with the SDK keyring, which only exposes safe methods, while here only the unsafe method is exposed. That said, since ExportPrivateKeyObject() was already part of the public API, this may not really matter.

Copy link
Contributor

@kristovatlas kristovatlas left a comment

Choose a reason for hiding this comment

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

I spent about an hour reviewing, and this all looks good to me to improving the interface and code clarity.

@zivkovicmilos zivkovicmilos merged commit 5a361f7 into master Dec 18, 2024
113 checks passed
@zivkovicmilos zivkovicmilos deleted the dev/zivkovicmilos/keybase-cleanup branch December 18, 2024 12:18
omarsy pushed a commit to omarsy/gno that referenced this pull request Dec 18, 2024
## Description

This PR greatly simplifies the `Keybase` interface in regards to key
imports / exports, by removing redundant methods.

The goal of the PR is to put the application-level key armor management
outside the keybase (not including storage), towards the caller. In
fact, this is the reason why the `Keybase` has a chaotic API for imports
/ exports.

I've preserved existing `gnokey` functionality -- this is unchanged.
I will update `gnokey` imports / exports in a separate PR, to make the
flag usage make more sense.

**BREAKING CHANGE:**
- Removed 9 `Keybase` methods, and added 2 new ones (that replace them)

cc @jefft0 

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] 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
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Functionality that contains breaking changes in focus Core team is prioritizing this work 📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Development

Successfully merging this pull request may close these issues.

8 participants