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

chore: improve error message for 'dfx cycles convert'. #4019

Merged
merged 10 commits into from
Dec 4, 2024

Conversation

vincent-dfinity
Copy link
Contributor

@vincent-dfinity vincent-dfinity commented Nov 27, 2024

Description

If users run dfx cycles convert without enough ICP tokens, show additional messages to indicate what to do next.

Error: Failed to transfer funds.
Caused by: the debit account doesn't have enough funds to complete the transaction, current balance: 0.00000000 ICP
Caused by: Diagnosis was added here.
Error explanation:
Insufficient ICP balance to finish the transfer transaction.
How to resolve the error:
Please top up your ICP balance.

Your account address for receiving ICP from centralized exchanges: 8494c01329531c06254ff45dad87db806ae6ed935ad6a504cdbc00a935db7b49
(run `dfx ledger account-id` to display)

Your principal for ICP wallets and decentralized exchanges: ueuar-wxbnk-bdcsr-dnrh3-rsyq6-ffned-h64ox-vxywi-gzawf-ot4pv-sqe
(run `dfx identity get-principal` to display)

Actually, it's not just an issue with dfx cycles convert, if you call dfx ledger transfer you will hit the same error. So we fixed it at the ledger function call level.

Fixes # (issue)
SDK-1868

How Has This Been Tested?

A new e2e test added.

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@vincent-dfinity vincent-dfinity marked this pull request as ready for review November 27, 2024 07:19
@vincent-dfinity vincent-dfinity requested a review from a team as a code owner November 27, 2024 07:19
Copy link
Member

@ericswanson-dfinity ericswanson-dfinity left a comment

Choose a reason for hiding this comment

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

The linked ticket includes a recommendation for what messages to display. This includes displaying both the ledger account id and the identity's principal, in addition to the commands to display these. (note that neither of the listed commands has an option to generate a QR code, though).

@vincent-dfinity
Copy link
Contributor Author

The linked ticket includes a recommendation for what messages to display. This includes displaying both the ledger account id and the identity's principal, in addition to the commands to display these. (note that neither of the listed commands has an option to generate a QR code, though).

Fixed at 1eacf6b.

@vincent-dfinity vincent-dfinity marked this pull request as draft December 1, 2024 15:29
@vincent-dfinity
Copy link
Contributor Author

@sesi200 @ericswanson-dfinity For now the diagnosis error looks like pretty much as we expected

dfx cycles convert --amount=0.123 --ic
Error: Failed to transfer funds.
Caused by: the debit account doesn't have enough funds to complete the transaction, current balance: 0.00000000 ICP
Caused by: Diagnosis was added here.
Error explanation:
Insufficient ICP balance to finish the transfer transaction.
How to resolve the error:
Please top up your ICP balance.

Your account address for receiving ICP from centralized exchanges: 8494c01329531c06254ff45dad87db806ae6ed935ad6a504cdbc00a935db7b49
(run `dfx ledger account-id` to display)

Your principal for ICP wallets and decentralized exchanges:ueuar-wxbnk-bdcsr-dnrh3-rsyq6-ffned-h64ox-vxywi-gzawf-ot4pv-sqe
(run `dfx identity get-principal` to display)

The only problem is about Caused by: Diagnosis was added here., I cannot find a way to remove it.

Is it possible to remove #[error("Diagnosis was added here.")] as it won't help on figuring out anything from it?

@sesi200
Copy link
Contributor

sesi200 commented Dec 2, 2024

I think we should remove #[context("Diagnosis was added here")] as it's not helping.

The other option to removing it is to explicitly filter it out here, but then we'd just filter out the only use of that context error. Better remove it entirely

@vincent-dfinity
Copy link
Contributor Author

I think we should remove #[context("Diagnosis was added here")] as it's not helping.

The other option to removing it is to explicitly filter it out here, but then we'd just filter out the only use of that context error. Better remove it entirely

Thanks. I've removed it at e9a127a. One thing is that it still inserts an empty error string, so I have to filter it out at where you pointed out.

@vincent-dfinity vincent-dfinity marked this pull request as ready for review December 3, 2024 11:46
CHANGELOG.md Outdated Show resolved Hide resolved
@vincent-dfinity vincent-dfinity merged commit 869f15a into master Dec 4, 2024
298 checks passed
@vincent-dfinity vincent-dfinity deleted the vincent/SDK-1868 branch December 4, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants