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

test(bug): In TestDeleteByAddress, delete by address, check deletion of all keys with that address #2476

Closed

Conversation

jefft0
Copy link
Contributor

@jefft0 jefft0 commented Jul 1, 2024

This PR demonstrates a bug. In keybase_test, we add a new test TestDeleteByAddress which is similar to the existing TestKeyManagement. We use CreateAccount to add two accounts with the same mnemonic (same bech32 address) but two different names, "john" and "john2". Then we call Delete to delete by the bech32 address. Only one of the keys is deleted. The other still exists.

To run the test:

cd gno/tm2/pkg/crypto/keys
go test .

Expected: pass. Actual: The test fails with:

        	Error Trace:	/Users/jefft0/work/gno/gno/tm2/pkg/crypto/keys/keybase_test.go:150
        	Error:      	Should be false
        	Test:       	TestDeleteByAddress

Discussion: Keybase writeInfo stores the same info under the key name and also the key address. This only allows an address to map to one name. When the key (with the same address) is created by the second name, the mapping from the address to the first name is replaced. Therefore, the call to delete by address only deletes the entry by the second name. The key is still accessible by the first name, meaning that "delete" doesn't really delete.

Possible solutions:

  • writeInfo should fail if there is already an entry for the address, or
  • writeInfo should map an address to a set of key names, or
  • don't "double index" by both name and address. (Only by name.)

@jefft0 jefft0 requested review from jaekwon, moul and a team as code owners July 1, 2024 15:45
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jul 1, 2024
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@jefft0 jefft0 added the 🐞 bug Something isn't working label Jul 1, 2024
@jefft0
Copy link
Contributor Author

jefft0 commented Jul 1, 2024

I added the "bug" label so that this PR gets attention as demonstrating a bug.

@jefft0 jefft0 changed the title chore: In keybase_test, add TestDeleteByAddress bug: In keybase_test, add TestDeleteByAddress Jul 3, 2024
@jefft0 jefft0 changed the title bug: In keybase_test, add TestDeleteByAddress bug: In TestDeleteByAddress, deleting by address should delete all keys with that address Jul 3, 2024
@jefft0 jefft0 changed the title bug: In TestDeleteByAddress, deleting by address should delete all keys with that address bug: In TestDeleteByAddress, delete by address, check deletion of all keys with that address Jul 3, 2024
@jefft0 jefft0 changed the title bug: In TestDeleteByAddress, delete by address, check deletion of all keys with that address test(bug): In TestDeleteByAddress, delete by address, check deletion of all keys with that address Jul 3, 2024
@Kouteki Kouteki added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 3, 2024
@jefft0 jefft0 removed request for a team, jaekwon and moul November 25, 2024 14:59
@jefft0
Copy link
Contributor Author

jefft0 commented Nov 27, 2024

Close in favor of the solution in PR #3221 .

@jefft0 jefft0 closed this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 📦 🌐 tendermint v2 Issues or PRs tm2 related review/triage-pending PRs opened by external contributors that are waiting for the 1st review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants