From e17ec66e3010b61fed47d3851542b0a48c1bd8db Mon Sep 17 00:00:00 2001 From: Vincent Zhang Date: Wed, 27 Nov 2024 15:13:08 +0800 Subject: [PATCH 1/7] Improve error message for 'dfx cycles convert'. --- CHANGELOG.md | 11 +++++++++++ e2e/tests-dfx/cycles-ledger.bash | 4 ++++ src/dfx/src/lib/diagnosis.rs | 18 ++++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5396853a28..45ec3dd715 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,17 @@ Added a flag `--replica` to `dfx start`. This flag currently has no effect. Once PocketIC becomes the default for `dfx start` this flag will start the replica instead. You can use the `--replica` flag already to write scripts that anticipate that change. +### 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. +Please run 'dfx ledger account-id' to get the address for receiving ICP tokens. +``` + # 0.24.3 ### feat: Bitcoin support in PocketIC diff --git a/e2e/tests-dfx/cycles-ledger.bash b/e2e/tests-dfx/cycles-ledger.bash index 7c20cbf057..a6091710a7 100644 --- a/e2e/tests-dfx/cycles-ledger.bash +++ b/e2e/tests-dfx/cycles-ledger.bash @@ -878,6 +878,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")" diff --git a/src/dfx/src/lib/diagnosis.rs b/src/dfx/src/lib/diagnosis.rs index 150f1c9b56..947bd8aaf2 100644 --- a/src/dfx/src/lib/diagnosis.rs +++ b/src/dfx/src/lib/diagnosis.rs @@ -1,4 +1,5 @@ use crate::lib::error_code; +use crate::lib::ledger_types::TransferError as ICPTransferError; use anyhow::Error as AnyhowError; use dfx_core::error::root_key::FetchRootKeyError; use ic_agent::agent::{RejectCode, RejectResponse}; @@ -72,6 +73,12 @@ pub fn diagnose(err: &AnyhowError) -> Diagnosis { } } + if let Some(transfer_error) = err.downcast_ref::() { + if insufficient_icp(transfer_error) { + return diagnose_insufficient_icp(); + } + } + NULL_DIAGNOSIS } @@ -262,3 +269,14 @@ fn diagnose_ledger_not_found() -> Diagnosis { (Some(explanation.to_string()), Some(suggestion.to_string())) } + +fn insufficient_icp(err: &ICPTransferError) -> bool { + matches!(err, ICPTransferError::InsufficientFunds { balance: _ }) +} + +fn diagnose_insufficient_icp() -> Diagnosis { + let explanation = "Insufficient ICP balance to finish the transfer transaction."; + let suggestion = "Please top up your ICP balance. +Please run 'dfx ledger account-id' to get the address for receiving ICP tokens."; + (Some(explanation.to_string()), Some(suggestion.to_string())) +} From 1eacf6bfba62ed26eb0ec87d52bd930d73b96556 Mon Sep 17 00:00:00 2001 From: Vincent Zhang Date: Wed, 27 Nov 2024 16:03:52 +0800 Subject: [PATCH 2/7] Improve the diagnosis message. --- CHANGELOG.md | 7 ++++++- src/dfx/src/lib/diagnosis.rs | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4663b6734..da1d97aba2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,7 +45,12 @@ Error explanation: Insufficient ICP balance to finish the transfer transaction. How to resolve the error: Please top up your ICP balance. -Please run 'dfx ledger account-id' to get the address for receiving ICP tokens. + +Please run 'dfx ledger account-id' to get your account address for receiving ICP from centralized exchanges. +(Example: a9e20b8d042cb7b73144de94e9091e8f2620dfc1e617cbfaa19d624322451b9d) + +Please run 'dfx identity get-principal' to get your principal for receiving ICP from ICP wallets and decentralized exchanges. +(Exmaple: treln-6wg26-lak4v-sh7p6-qm7hn-ohcjl-ut3os-lemuf-ypxvr-zcamm-gae) ``` # 0.24.3 diff --git a/src/dfx/src/lib/diagnosis.rs b/src/dfx/src/lib/diagnosis.rs index 947bd8aaf2..47119c9878 100644 --- a/src/dfx/src/lib/diagnosis.rs +++ b/src/dfx/src/lib/diagnosis.rs @@ -277,6 +277,11 @@ fn insufficient_icp(err: &ICPTransferError) -> bool { fn diagnose_insufficient_icp() -> Diagnosis { let explanation = "Insufficient ICP balance to finish the transfer transaction."; let suggestion = "Please top up your ICP balance. -Please run 'dfx ledger account-id' to get the address for receiving ICP tokens."; + +Please run 'dfx ledger account-id' to get your account address for receiving ICP from centralized exchanges. +(Example: a9e20b8d042cb7b73144de94e9091e8f2620dfc1e617cbfaa19d624322451b9d) + +Please run 'dfx identity get-principal' to get your principal for receiving ICP from ICP wallets and decentralized exchanges. +(Exmaple: treln-6wg26-lak4v-sh7p6-qm7hn-ohcjl-ut3os-lemuf-ypxvr-zcamm-gae)"; (Some(explanation.to_string()), Some(suggestion.to_string())) } From 40396988c1b53884df7ad7c18f686b3cbbf628cc Mon Sep 17 00:00:00 2001 From: Vincent Zhang Date: Sun, 1 Dec 2024 23:33:36 +0800 Subject: [PATCH 3/7] Catch the error deeper and compose the DiagnosedError. --- src/dfx/src/lib/operations/ledger.rs | 34 ++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/dfx/src/lib/operations/ledger.rs b/src/dfx/src/lib/operations/ledger.rs index d48047ea66..426906da1d 100644 --- a/src/dfx/src/lib/operations/ledger.rs +++ b/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::{ @@ -142,6 +143,9 @@ pub async fn transfer( info!(logger, "{}", TransferError::TxDuplicate { duplicate_of }); break duplicate_of; } + Err(TransferError::InsufficientFunds { balance }) => { + return compose_insufficient_funds_error(agent, from_subaccount, balance); + } Err(transfer_err) => bail!(transfer_err), } } @@ -164,6 +168,36 @@ pub async fn transfer( Ok(block_height) } +fn compose_insufficient_funds_error( + agent: &Agent, + subaccount: Option, + balance: ICPTs, +) -> DfxResult { + 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() + ); + + Err(DiagnosedError::new( + explanation.to_string(), + suggestion + )).context(format!( + "the debit account doesn't have enough funds to complete the transaction, current balance: {}", + balance + )) +} + fn retryable(agent_error: &AgentError) -> bool { match agent_error { AgentError::CertifiedReject(RejectResponse { From 27d5c719875cc9191bd510adbdeb08c6009536f2 Mon Sep 17 00:00:00 2001 From: Vincent Zhang Date: Mon, 2 Dec 2024 15:08:00 +0800 Subject: [PATCH 4/7] Discard the changes in 'diagnosis.rs'. --- src/dfx/src/lib/diagnosis.rs | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/src/dfx/src/lib/diagnosis.rs b/src/dfx/src/lib/diagnosis.rs index 86e387fe06..b346341cf7 100644 --- a/src/dfx/src/lib/diagnosis.rs +++ b/src/dfx/src/lib/diagnosis.rs @@ -1,6 +1,5 @@ use crate::lib::cycles_ledger_types::create_canister::CreateCanisterError; use crate::lib::error_code; -use crate::lib::ledger_types::TransferError as ICPTransferError; use anyhow::Error as AnyhowError; use dfx_core::error::root_key::FetchRootKeyError; use dfx_core::network::provider::get_network_context; @@ -81,12 +80,6 @@ pub fn diagnose(err: &AnyhowError) -> Diagnosis { } } - if let Some(transfer_error) = err.downcast_ref::() { - if insufficient_icp(transfer_error) { - return diagnose_insufficient_icp(); - } - } - NULL_DIAGNOSIS } @@ -302,19 +295,3 @@ fn diagnose_insufficient_cycles() -> Diagnosis { ); (Some(explanation.to_string()), Some(suggestion)) } - -fn insufficient_icp(err: &ICPTransferError) -> bool { - matches!(err, ICPTransferError::InsufficientFunds { balance: _ }) -} - -fn diagnose_insufficient_icp() -> Diagnosis { - let explanation = "Insufficient ICP balance to finish the transfer transaction."; - let suggestion = "Please top up your ICP balance. - -Please run 'dfx ledger account-id' to get your account address for receiving ICP from centralized exchanges. -(Example: a9e20b8d042cb7b73144de94e9091e8f2620dfc1e617cbfaa19d624322451b9d) - -Please run 'dfx identity get-principal' to get your principal for receiving ICP from ICP wallets and decentralized exchanges. -(Exmaple: treln-6wg26-lak4v-sh7p6-qm7hn-ohcjl-ut3os-lemuf-ypxvr-zcamm-gae)"; - (Some(explanation.to_string()), Some(suggestion.to_string())) -} From e9a127af576a18854adda123e82097302f83dc79 Mon Sep 17 00:00:00 2001 From: Vincent Zhang Date: Tue, 3 Dec 2024 19:41:01 +0800 Subject: [PATCH 5/7] Remove context error message from DiagnosedError. --- src/dfx/src/lib/diagnosis.rs | 10 ++++++---- src/dfx/src/main.rs | 4 ++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/dfx/src/lib/diagnosis.rs b/src/dfx/src/lib/diagnosis.rs index b346341cf7..d90a133f11 100644 --- a/src/dfx/src/lib/diagnosis.rs +++ b/src/dfx/src/lib/diagnosis.rs @@ -17,10 +17,6 @@ pub type Diagnosis = (Option, Option); 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 { /// A user-friendly explanation of what went wrong. pub error_explanation: Option, @@ -29,6 +25,12 @@ pub struct DiagnosedError { pub action_suggestion: Option, } +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 { diff --git a/src/dfx/src/main.rs b/src/dfx/src/main.rs index 79a19d3542..85603b4657 100644 --- a/src/dfx/src/main.rs +++ b/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 { From fe88fecd4604f79b10f37824f4508d0028367aa3 Mon Sep 17 00:00:00 2001 From: Vincent Zhang Date: Tue, 3 Dec 2024 20:04:51 +0800 Subject: [PATCH 6/7] Update changelog. --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cfe83a72c..d8dec8e193 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,11 +57,11 @@ Insufficient ICP balance to finish the transfer transaction. How to resolve the error: Please top up your ICP balance. -Please run 'dfx ledger account-id' to get your account address for receiving ICP from centralized exchanges. -(Example: a9e20b8d042cb7b73144de94e9091e8f2620dfc1e617cbfaa19d624322451b9d) +Your account address for receiving ICP from centralized exchanges: 8494c01329531c06254ff45dad87db806ae6ed935ad6a504cdbc00a935db7b49 +(run `dfx ledger account-id` to display) -Please run 'dfx identity get-principal' to get your principal for receiving ICP from ICP wallets and decentralized exchanges. -(Exmaple: treln-6wg26-lak4v-sh7p6-qm7hn-ohcjl-ut3os-lemuf-ypxvr-zcamm-gae) +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 From 0c9b12feb620103990f5033b8163447e3122748d Mon Sep 17 00:00:00 2001 From: Vincent Zhang Date: Wed, 4 Dec 2024 07:51:49 +0800 Subject: [PATCH 7/7] Addressed review comments. --- CHANGELOG.md | 2 +- src/dfx/src/lib/diagnosis.rs | 2 ++ src/dfx/src/lib/operations/ledger.rs | 22 +++++++++------------- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8dec8e193..16d3505118 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,7 +60,7 @@ 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 +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) ``` diff --git a/src/dfx/src/lib/diagnosis.rs b/src/dfx/src/lib/diagnosis.rs index d90a133f11..49bd3b2b77 100644 --- a/src/dfx/src/lib/diagnosis.rs +++ b/src/dfx/src/lib/diagnosis.rs @@ -17,6 +17,8 @@ pub type Diagnosis = (Option, Option); pub const NULL_DIAGNOSIS: Diagnosis = (None, None); #[derive(ThisError, Debug)] +/// 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 { /// A user-friendly explanation of what went wrong. pub error_explanation: Option, diff --git a/src/dfx/src/lib/operations/ledger.rs b/src/dfx/src/lib/operations/ledger.rs index 426906da1d..37f882ea79 100644 --- a/src/dfx/src/lib/operations/ledger.rs +++ b/src/dfx/src/lib/operations/ledger.rs @@ -9,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}; @@ -144,7 +144,10 @@ pub async fn transfer( break duplicate_of; } Err(TransferError::InsufficientFunds { balance }) => { - return compose_insufficient_funds_error(agent, from_subaccount, balance); + return Err(anyhow!(TransferError::InsufficientFunds { balance })) + .with_context(|| { + diagnose_insufficient_funds_error(agent, from_subaccount) + }); } Err(transfer_err) => bail!(transfer_err), } @@ -168,11 +171,10 @@ pub async fn transfer( Ok(block_height) } -fn compose_insufficient_funds_error( +fn diagnose_insufficient_funds_error( agent: &Agent, subaccount: Option, - balance: ICPTs, -) -> DfxResult { +) -> DiagnosedError { let principal = agent.get_principal().unwrap(); // This should always succeed at this point. let explanation = "Insufficient ICP balance to finish the transfer transaction."; @@ -182,20 +184,14 @@ fn compose_insufficient_funds_error( Your account address for receiving ICP from centralized exchanges: {} (run `dfx ledger account-id` to display) -Your principal for ICP wallets and decentralized exchanges:{} +Your principal for ICP wallets and decentralized exchanges: {} (run `dfx identity get-principal` to display) ", AccountIdentifier::new(principal, subaccount), principal.to_text() ); - Err(DiagnosedError::new( - explanation.to_string(), - suggestion - )).context(format!( - "the debit account doesn't have enough funds to complete the transaction, current balance: {}", - balance - )) + DiagnosedError::new(explanation.to_string(), suggestion) } fn retryable(agent_error: &AgentError) -> bool {