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!: key rename cli command #9601

Merged
merged 29 commits into from
Jul 19, 2021
Merged

feat!: key rename cli command #9601

merged 29 commits into from
Jul 19, 2021

Conversation

technicallyty
Copy link
Contributor

@technicallyty technicallyty commented Jun 28, 2021

Description

this PR adds a new function to the keyring interface, as well as a CLI command to rename a key in the keyring

ref: #9407


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added C:CLI C:Keys Keybase, KMS and HSMs labels Jun 28, 2021
@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #9601 (4edd229) into master (0027111) will increase coverage by 27.94%.
The diff coverage is 63.79%.

❗ Current head 4edd229 differs from pull request most recent head c21bd5f. Consider uploading reports for the commit c21bd5f to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9601       +/-   ##
===========================================
+ Coverage   35.48%   63.42%   +27.94%     
===========================================
  Files         332      574      +242     
  Lines       32620    37648     +5028     
===========================================
+ Hits        11575    23880    +12305     
+ Misses      19819    11900     -7919     
- Partials     1226     1868      +642     
Impacted Files Coverage Δ
client/keys/types.go 100.00% <ø> (+100.00%) ⬆️
client/query.go 16.98% <ø> (ø)
client/rpc/block.go 0.00% <ø> (ø)
client/rpc/status.go 67.74% <ø> (ø)
client/rpc/validators.go 0.00% <ø> (ø)
client/test_helpers.go 0.00% <ø> (ø)
client/tx/factory.go 27.00% <ø> (ø)
client/tx/legacy.go 68.42% <ø> (ø)
client/tx/tx.go 40.83% <ø> (ø)
client/utils.go 41.93% <ø> (-41.40%) ⬇️
... and 686 more

Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

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

left some nits and also could you resolve the conflicts ?

CHANGELOG.md Outdated Show resolved Hide resolved
client/keys/rename.go Outdated Show resolved Hide resolved
client/keys/rename.go Outdated Show resolved Hide resolved
client/keys/rename.go Outdated Show resolved Hide resolved
client/keys/rename.go Outdated Show resolved Hide resolved
crypto/keyring/keyring_test.go Outdated Show resolved Hide resolved
crypto/keyring/keyring_test.go Outdated Show resolved Hide resolved
crypto/keyring/keyring_test.go Outdated Show resolved Hide resolved
crypto/keyring/keyring_test.go Outdated Show resolved Hide resolved
crypto/keyring/keyring_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Manually tested. Seems to be working without issues. A couple comments.

crypto/keyring/keyring.go Outdated Show resolved Hide resolved
@@ -1153,6 +1153,52 @@ func TestBackendConfigConstructors(t *testing.T) {
require.Equal(t, "keyring-test", backend.PassPrefix)
}

func TestRenameKey(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For each test, should we check more than just an error? Should we also check whether each key does or does not exist within the keyring after renaming?

Copy link
Contributor Author

@technicallyty technicallyty Jul 5, 2021

Choose a reason for hiding this comment

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

Should we also check whether each key does or does not exist within the keyring after renaming?

this is already checked on L1175 and L1182-L1183

Copy link
Contributor

Choose a reason for hiding this comment

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

Within this test, rename is called three times at the end without checking the specific error thrown or what keys exist after each failed attempt. I was thinking the extra checks would be added to the failed attempts. At the least, I think we should check to make sure we are receiving the expected error and not just any error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored the tests. should be a bit more concise. let me know if its missing anything please!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I like the direction with adding test cases. I'm not sure if it's necessary given that the only parameter for each test case is the run function and there is no overlap with the key names you are using so creating a new keyring for each test case might not be necessary but I also think it's ok to leave it as is.

crypto/keyring/keyring.go Outdated Show resolved Hide resolved
return errors.New(fmt.Sprintf("rename failed: %s already exists in the keyring", newName))
}

passPhrase := "temp"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this should be a private const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup that makes much more sense - moved it to the other private const vars

Comment on lines 446 to 449
err = ks.Delete(oldName)
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called after ImportPrivKey? What happens if ImportPrivKey fails? The account is deleted and no new account is created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so import will actually fail if you delete last, cause import doesn't allow same pubkeys. some solutions i can think of:

  1. dump the key information in a recovery file

  2. update the ImportPrivKey method to allow keys with same pubkeys, but different keyring names

thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the name is the primary index, I think it's OK to go with (2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, changed the write behavior to allow duplicate keys, as long as the names are different.

Copy link
Contributor

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

tACK. No necessary changes but I left a few comments.

CHANGELOG.md Outdated Show resolved Hide resolved
crypto/keyring/keyring.go Outdated Show resolved Hide resolved
crypto/keyring/keyring_test.go Show resolved Hide resolved
@@ -1153,6 +1153,52 @@ func TestBackendConfigConstructors(t *testing.T) {
require.Equal(t, "keyring-test", backend.PassPrefix)
}

func TestRenameKey(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I like the direction with adding test cases. I'm not sure if it's necessary given that the only parameter for each test case is the run function and there is no overlap with the key names you are using so creating a new keyring for each test case might not be necessary but I also think it's ok to leave it as is.

crypto/keyring/keyring_test.go Show resolved Hide resolved
technicallyty and others added 2 commits July 8, 2021 12:15
Co-authored-by: Ryan Christoffersen <[email protected]>
Co-authored-by: Ryan Christoffersen <[email protected]>
Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

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

lgtm

@technicallyty technicallyty added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 9, 2021
@technicallyty
Copy link
Contributor Author

the change request is blocking merge @alexanderbez, could you re-review when you have time?

@technicallyty
Copy link
Contributor Author

@alexanderbez ping - still blocked 🙈

@technicallyty technicallyty changed the title feat: key rename cli command feat!: key rename cli command Jul 18, 2021
@technicallyty technicallyty added A:automerge Automatically merge PR once all prerequisites pass. and removed A:automerge Automatically merge PR once all prerequisites pass. labels Jul 19, 2021
@mergify mergify bot merged commit 57d21fa into master Jul 19, 2021
@mergify mergify bot deleted the ty/9407-rename_keys_cli branch July 19, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:Keys Keybase, KMS and HSMs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants