Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

Problem: Ambiguous error message for empty staking address #845

Merged
merged 1 commit into from
Jan 8, 2020
Merged

Problem: Ambiguous error message for empty staking address #845

merged 1 commit into from
Jan 8, 2020

Conversation

devashishdxt
Copy link
Collaborator

Solution: Added a more detailed error message. Fixes #838

Solution: Added a more detailed error message. Fixes #838
@@ -126,7 +126,7 @@ impl fmt::Display for Error {
// FIXME: IoError(ref err) => write!(f, "IO error: {}", err),
IoError => write!(f, "database lookup error"),
EnclaveRejected => write!(f, "enclave error or invalid TX"),
AccountNotFound => write!(f, "account not found"),
AccountNotFound => write!(f, "account does not exist for given staking address on blockchain"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess the main confusion in the issue is, the staking address do exists in the wallet, it just has empty transaction history on the blockchain, so a better solution maybe show a empty value to user. A default StakingState when query staking state, a empty list when query transaction history.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do not any command to show transaction history for a staking account. Only fetching current state is supported. When an account does not exist, we have 2 options:

  1. Show a more detailed error message that the account for given staking address does not exist on blockchain.
  2. Show a default state.

After discussing with @tomtau and @lezzokafka, we reached on conclusion to use option 1. But, we can show a default state if that seems to be more correct option.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👌

@codecov
Copy link

codecov bot commented Jan 7, 2020

Codecov Report

Merging #845 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #845      +/-   ##
==========================================
- Coverage   69.17%   69.17%   -0.01%     
==========================================
  Files         132      132              
  Lines       17184    17187       +3     
==========================================
+ Hits        11887    11889       +2     
- Misses       5297     5298       +1
Impacted Files Coverage Δ
chain-tx-validation/src/lib.rs 75.53% <0%> (ø) ⬆️
client-core/src/wallet/syncer.rs 61% <0%> (-0.28%) ⬇️
chain-core/src/common/merkle_tree.rs 98.79% <0%> (+0.24%) ⬆️

@tomtau
Copy link
Contributor

tomtau commented Jan 8, 2020

bors r+

bors bot added a commit that referenced this pull request Jan 8, 2020
845: Problem: Ambiguous error message for empty staking address r=tomtau a=devashishdxt

Solution: Added a more detailed error message. Fixes #838

Co-authored-by: Devashish Dixit <devashish@crypto.com>
@bors
Copy link
Contributor

bors bot commented Jan 8, 2020

@bors bors bot merged commit bfa5658 into crypto-com:master Jan 8, 2020
@devashishdxt devashishdxt deleted the account-not-found branch January 8, 2020 03:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem: ambiguous error message for empty staking address with client-cli
4 participants