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 TestExportImport, delete imported key, check if original exists #2472

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jefft0
Copy link
Contributor

@jefft0 jefft0 commented Jul 1, 2024

This PR demonstrates a bug. TestExportImport creates a key named "john", uses Export to export it, then uses Import to import it into the same Keybase with the new name "john2". It checks if the address of the imported key is the same as the original key.

This PR expands TestExportImport to delete the imported "john2" then uses HasByName("john") to check if the original key 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:246
        	Error:      	Should be true
        	Test:       	TestExportImport

Discussion: The Export function exports the Info struct which has the key name, in this case "john". Then Import imports the key with a new name, but does not change the Info struct. (Is this by design?) The Delete function fetches the Info struct from the database using the new name "john2", but it deletes based on the original name in the struct. This deletes the key under the original name.

Maybe Import needs to modify the Info struct with the new name before storing in the database.

@jefft0 jefft0 requested review from jaekwon, moul and a team as code owners July 1, 2024 07:45
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jul 1, 2024
@jefft0 jefft0 added 🐞 bug Something isn't working and removed 📦 🌐 tendermint v2 Issues or PRs tm2 related labels 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
Copy link
Contributor Author

jefft0 commented Jul 1, 2024

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

@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jul 1, 2024
@jefft0 jefft0 changed the title chore: In TestExportImport, delete imported key, check if original exists bug: In TestExportImport, delete imported key, check if original exists Jul 3, 2024
@jefft0 jefft0 changed the title bug: In TestExportImport, delete imported key, check if original exists test(bug): In TestExportImport, delete imported key, check if original exists 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 15:00
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: No status
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants