fix: In dbKeybase.writeInfo, if replacing a name entry, remove the lookup by the old address #2685
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…okup by the old address. See the PR. Signed-off-by: Jeff Thompson <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2685 +/- ##
==========================================
- Coverage 60.22% 60.21% -0.01%
==========================================
Files 562 562
Lines 75038 75048 +10
==========================================
- Hits 45188 45187 -1
- Misses 26476 26482 +6
- Partials 3374 3379 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This was referenced Aug 13, 2024
Kouteki
added
the
review/triage-pending
notJoon
approved these changes
Oct 22, 2024
notJoon
removed
the
review/triage-pending
thehowl
approved these changes
Oct 22, 2024
6 tasks
zivkovicmilos
pushed a commit
that referenced
this pull request
Dec 6, 2024
The `Keybase` interface was written with the assumption of a one-to-one correspondence between a key's name and address. But this needs to be enforced by the code. PR #2685 updated `writeInfo` for the case of inserting a new key (address) with the same name as an existing key with a different address. In this case, `writeInfo` uses the name to look up the existing address and deletes the address entry. This PR does the same for the other case: Inserting a key with the same address as an existing key, but a new name. In this case, `writeInfo` uses the address to look up the existing key's name, and deletes the name entry. This is not a breaking change because none of the production code expects to add a key a second time with the same address but a different name. We update the `Keybase` doc comments to this effect. However, some of the tests in keybase_test.go make this assumption, so this PR fixes them: * `TestSignVerify` creates a key with name n2 and exports its public key. It wants to re-import the public key with name n3 and get the public key to check that it is the same public key as n2. We change this test to re-import the public key into a fresh in-memory key store. * `TestExportImportPubKey` creates a key with name "john", exports its public key, then re-imports it with the new name "john-pubkey-only" (but the same address). The current test checks that the key can still be fetched under the old name "john". But this breaks the one-to-one correspondence of key name and address. So the test is changed to confirm that the key with the old name is replaced by the new name. <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 - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests </details> Signed-off-by: Jeff Thompson <[email protected]>
omarsy
pushed a commit
to TERITORI/gno
that referenced
this pull request
Dec 7, 2024
The `Keybase` interface was written with the assumption of a one-to-one correspondence between a key's name and address. But this needs to be enforced by the code. PR gnolang#2685 updated `writeInfo` for the case of inserting a new key (address) with the same name as an existing key with a different address. In this case, `writeInfo` uses the name to look up the existing address and deletes the address entry. This PR does the same for the other case: Inserting a key with the same address as an existing key, but a new name. In this case, `writeInfo` uses the address to look up the existing key's name, and deletes the name entry. This is not a breaking change because none of the production code expects to add a key a second time with the same address but a different name. We update the `Keybase` doc comments to this effect. However, some of the tests in keybase_test.go make this assumption, so this PR fixes them: * `TestSignVerify` creates a key with name n2 and exports its public key. It wants to re-import the public key with name n3 and get the public key to check that it is the same public key as n2. We change this test to re-import the public key into a fresh in-memory key store. * `TestExportImportPubKey` creates a key with name "john", exports its public key, then re-imports it with the new name "john-pubkey-only" (but the same address). The current test checks that the key can still be fetched under the old name "john". But this breaks the one-to-one correspondence of key name and address. So the test is changed to confirm that the key with the old name is replaced by the new name. <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 - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests </details> Signed-off-by: Jeff Thompson <[email protected]>
Villaquiranm
pushed a commit
to Villaquiranm/gno
that referenced
this pull request
Dec 9, 2024
The `Keybase` interface was written with the assumption of a one-to-one correspondence between a key's name and address. But this needs to be enforced by the code. PR gnolang#2685 updated `writeInfo` for the case of inserting a new key (address) with the same name as an existing key with a different address. In this case, `writeInfo` uses the name to look up the existing address and deletes the address entry. This PR does the same for the other case: Inserting a key with the same address as an existing key, but a new name. In this case, `writeInfo` uses the address to look up the existing key's name, and deletes the name entry. This is not a breaking change because none of the production code expects to add a key a second time with the same address but a different name. We update the `Keybase` doc comments to this effect. However, some of the tests in keybase_test.go make this assumption, so this PR fixes them: * `TestSignVerify` creates a key with name n2 and exports its public key. It wants to re-import the public key with name n3 and get the public key to check that it is the same public key as n2. We change this test to re-import the public key into a fresh in-memory key store. * `TestExportImportPubKey` creates a key with name "john", exports its public key, then re-imports it with the new name "john-pubkey-only" (but the same address). The current test checks that the key can still be fetched under the old name "john". But this breaks the one-to-one correspondence of key name and address. So the test is changed to confirm that the key with the old name is replaced by the new name. <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 - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests </details> Signed-off-by: Jeff Thompson <[email protected]>
r3v4s
pushed a commit
to gnoswap-labs/gno
that referenced
this pull request
Dec 10, 2024
The `Keybase` interface was written with the assumption of a one-to-one correspondence between a key's name and address. But this needs to be enforced by the code. PR gnolang#2685 updated `writeInfo` for the case of inserting a new key (address) with the same name as an existing key with a different address. In this case, `writeInfo` uses the name to look up the existing address and deletes the address entry. This PR does the same for the other case: Inserting a key with the same address as an existing key, but a new name. In this case, `writeInfo` uses the address to look up the existing key's name, and deletes the name entry. This is not a breaking change because none of the production code expects to add a key a second time with the same address but a different name. We update the `Keybase` doc comments to this effect. However, some of the tests in keybase_test.go make this assumption, so this PR fixes them: * `TestSignVerify` creates a key with name n2 and exports its public key. It wants to re-import the public key with name n3 and get the public key to check that it is the same public key as n2. We change this test to re-import the public key into a fresh in-memory key store. * `TestExportImportPubKey` creates a key with name "john", exports its public key, then re-imports it with the new name "john-pubkey-only" (but the same address). The current test checks that the key can still be fetched under the old name "john". But this breaks the one-to-one correspondence of key name and address. So the test is changed to confirm that the key with the old name is replaced by the new name. <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 - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests </details> Signed-off-by: Jeff Thompson <[email protected]>
albttx
pushed a commit
that referenced
this pull request
Jan 10, 2025
The `Keybase` interface was written with the assumption of a one-to-one correspondence between a key's name and address. But this needs to be enforced by the code. PR #2685 updated `writeInfo` for the case of inserting a new key (address) with the same name as an existing key with a different address. In this case, `writeInfo` uses the name to look up the existing address and deletes the address entry. This PR does the same for the other case: Inserting a key with the same address as an existing key, but a new name. In this case, `writeInfo` uses the address to look up the existing key's name, and deletes the name entry. This is not a breaking change because none of the production code expects to add a key a second time with the same address but a different name. We update the `Keybase` doc comments to this effect. However, some of the tests in keybase_test.go make this assumption, so this PR fixes them: * `TestSignVerify` creates a key with name n2 and exports its public key. It wants to re-import the public key with name n3 and get the public key to check that it is the same public key as n2. We change this test to re-import the public key into a fresh in-memory key store. * `TestExportImportPubKey` creates a key with name "john", exports its public key, then re-imports it with the new name "john-pubkey-only" (but the same address). The current test checks that the key can still be fetched under the old name "john". But this breaks the one-to-one correspondence of key name and address. So the test is changed to confirm that the key with the old name is replaced by the new name. <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 - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests </details> Signed-off-by: Jeff Thompson <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the bug demonstrated in #2684 :
dbKeybase.writeInfo, add anerrorreturn and propagate this to all callers (writeLocalKey, etc.) . We needwriteInfoto do some database integrity checks, so it needs to be able to return an error.dbKeybase.writeInfoas suggested in #2684 . If an existing name entry is being replaced with newInfo, then remove the "lookup by address" entry for the existing address.TestKeyManagement, add a test similar to #2684 , except that after usingCreateAccountwith the same name, expectGetByAddressto fail for the obsolete address and its (non-existing) public key. (This test passes.)Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description