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

* Improve error message for 'dfx cycles convert'.

* Improve the diagnosis message.

* Catch the error deeper and compose the DiagnosedError.

* Discard the changes in 'diagnosis.rs'.

* Remove context error message from DiagnosedError.

* Update changelog.

* Addressed review comments.

Author: vincent-dfinity

CHANGELOG.md

@@ -48,6 +48,22 @@ Please top up your cycles balance by converting ICP to cycles like below:
 'dfx cycles convert --amount=0.123'.
 ```
 
+### chore: improve `dfx cycles convert` messages.
+
+If users run `dfx cycles convert` without enough ICP tokens, show additional messages to indicate what to do next.
+```
+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)
+```
+
 # 0.24.3
 
 ### feat: Bitcoin support in PocketIC

e2e/tests-dfx/cycles-ledger.bash

@@ -898,6 +898,10 @@ current_time_nanoseconds() {
 
   deploy_cycles_ledger
 
+  # Test failed to convert ICP to cycles without enough ICP.
+  assert_command_fail dfx cycles convert --amount 12.5 --identity alice
+  assert_contains "Insufficient ICP balance to finish the transfer transaction."
+
   assert_command dfx --identity cycle-giver ledger transfer --memo 1234 --amount 100 "$(dfx ledger account-id --of-principal "$ALICE")"
   assert_command dfx --identity cycle-giver ledger transfer --memo 1234 --amount 100 "$(dfx ledger account-id --of-principal "$ALICE" --subaccount "$ALICE_SUBACCT1")"
 

src/dfx/src/lib/diagnosis.rs

@@ -17,8 +17,6 @@ pub type Diagnosis = (Option<String>, Option<String>);
 pub const NULL_DIAGNOSIS: Diagnosis = (None, None);
 
 #[derive(ThisError, Debug)]
-// This message will appear in the context trace of the stack. The diagnosis should not be displayed there yet.
-#[error("Diagnosis was added here.")]
 /// If you do not need the generic error diagnosis to run, you can add a DiagnosedError with .context(err: DiagnosedError).
 /// In that case, no extra diagnosis is attempted and the last-added explanation and suggestion are printed out.
 pub struct DiagnosedError {
@@ -29,6 +27,12 @@ pub struct DiagnosedError {
     pub action_suggestion: Option<String>,
 }
 
+impl std::fmt::Display for DiagnosedError {
+    fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        Ok(())
+    }
+}
+
 impl DiagnosedError {
     pub fn new(error_explanation: String, action_suggestion: String) -> Self {
         Self {

src/dfx/src/lib/operations/ledger.rs

@@ -1,3 +1,4 @@
+use crate::lib::diagnosis::DiagnosedError;
 use crate::lib::ledger_types::{AccountIdBlob, BlockHeight, Memo, TransferError};
 use crate::lib::nns_types::account_identifier::Subaccount;
 use crate::lib::{
@@ -8,7 +9,7 @@ use crate::lib::{
     },
     nns_types::{account_identifier::AccountIdentifier, icpts::ICPTs},
 };
-use anyhow::{bail, ensure, Context};
+use anyhow::{anyhow, bail, ensure, Context};
 use backoff::backoff::Backoff;
 use backoff::ExponentialBackoff;
 use candid::{Decode, Encode, Principal};
@@ -142,6 +143,12 @@ pub async fn transfer(
                         info!(logger, "{}", TransferError::TxDuplicate { duplicate_of });
                         break duplicate_of;
                     }
+                    Err(TransferError::InsufficientFunds { balance }) => {
+                        return Err(anyhow!(TransferError::InsufficientFunds { balance }))
+                            .with_context(|| {
+                                diagnose_insufficient_funds_error(agent, from_subaccount)
+                            });
+                    }
                     Err(transfer_err) => bail!(transfer_err),
                 }
             }
@@ -164,6 +171,29 @@ pub async fn transfer(
     Ok(block_height)
 }
 
+fn diagnose_insufficient_funds_error(
+    agent: &Agent,
+    subaccount: Option<Subaccount>,
+) -> DiagnosedError {
+    let principal = agent.get_principal().unwrap(); // This should always succeed at this point.
+
+    let explanation = "Insufficient ICP balance to finish the transfer transaction.";
+    let suggestion = format!(
+        "Please top up your ICP balance.
+
+Your account address for receiving ICP from centralized exchanges: {}
+(run `dfx ledger account-id` to display)
+
+Your principal for ICP wallets and decentralized exchanges: {}
+(run `dfx identity get-principal` to display)
+",
+        AccountIdentifier::new(principal, subaccount),
+        principal.to_text()
+    );
+
+    DiagnosedError::new(explanation.to_string(), suggestion)
+}
+
 fn retryable(agent_error: &AgentError) -> bool {
     match agent_error {
         AgentError::CertifiedReject(RejectResponse {

src/dfx/src/main.rs

@@ -72,6 +72,10 @@ fn print_error_and_diagnosis(err: Error, error_diagnosis: Diagnosis) {
 
     // print error chain stack
     for (level, cause) in err.chain().enumerate() {
+        if cause.to_string().is_empty() {
+            continue;
+        }
+
         let (color, prefix) = if level == 0 {
             (term::color::RED, "Error")
         } else {