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
16 changes: 16 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions e2e/tests-dfx/cycles-ledger.bash
Original file line number Diff line number Diff line change
Expand Up @@ -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")"

Expand Down
8 changes: 6 additions & 2 deletions src/dfx/src/lib/diagnosis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
32 changes: 31 additions & 1 deletion src/dfx/src/lib/operations/ledger.rs
Original file line number Diff line number Diff line change
@@ -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::{
Expand All @@ -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};
Expand Down Expand Up @@ -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),
}
}
Expand All @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions src/dfx/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading