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

wallet store decoding error is misleading #2151

Closed
tzemanovic opened this issue Nov 11, 2023 · 6 comments · Fixed by nodersteam/namada#1 or #3705
Closed

wallet store decoding error is misleading #2151

tzemanovic opened this issue Nov 11, 2023 · 6 comments · Fixed by nodersteam/namada#1 or #3705
Assignees
Labels
bug Something isn't working cli wallet

Comments

@tzemanovic
Copy link
Member

When the format of the wallet.toml is incorrect, running commands that load the wallet fail with a misleading error (e.g. No pre-genesis wallet found at ...) instead reporting decoding error

@tzemanovic tzemanovic added bug Something isn't working cli wallet labels Nov 11, 2023
@Kofituo
Copy link
Contributor

Kofituo commented Jan 11, 2024

Hello. I'm working on this issue. I'd like more information about it. How can I reproduce this error?
I see the error is related to this method:

pub fn load_pre_genesis_wallet_or_exit(
base_dir: &Path,
) -> (Wallet<CliWalletUtils>, PathBuf) {
try_load_pre_genesis_wallet(base_dir).unwrap_or_else(|| {
eprintln!("No pre-genesis wallet found.",);
safe_exit(1)
})
}

@tzemanovic
Copy link
Member Author

hi @Kofituo ! It looks like the behavior has slightly changed in more recent versions. You can reproduce it by manually editing a wallet file ({base_dir}/pre-genesis/wallet.toml}) to contains some invalid contents. Using a wallet command (e.g. namadaw --pre-genesis list) doesn't seem to print anything on latest release v0.29. For a chain wallet (e.g. {base_dir}/{chain_id}/wallet.toml}) the command panics on unwrap of None

@Kofituo
Copy link
Contributor

Kofituo commented Jan 11, 2024

@tzemanovic thanks for the feedback. I'll give it a try. Could you kindly take a look at the linked pr as well?

@tzemanovic
Copy link
Member Author

@Kofituo the PR lgtm!

nodersteam pushed a commit to nodersteam/namada that referenced this issue Jan 15, 2024
nodersteam pushed a commit to nodersteam/namada that referenced this issue Jan 15, 2024
@cwgoes
Copy link
Contributor

cwgoes commented Aug 27, 2024

@tzemanovic Is this still applicable? Should we merge the linked PR on the fork here?

@cwgoes cwgoes added this to the Nice to have and non-breaking milestone Aug 27, 2024
@tzemanovic
Copy link
Member Author

@tzemanovic Is this still applicable? Should we merge the linked PR on the fork here?

yeah, will do

@tzemanovic tzemanovic self-assigned this Aug 27, 2024
tzemanovic pushed a commit that referenced this issue Aug 27, 2024
tzemanovic added a commit that referenced this issue Aug 27, 2024
@tzemanovic tzemanovic mentioned this issue Aug 27, 2024
1 task
@mergify mergify bot closed this as completed in #3705 Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment