-
Notifications
You must be signed in to change notification settings - Fork 18
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
[BLOCKBACK-162] Error message correction #190
Conversation
We merged |
@oxarbitrage Thank you. @pbattu123 suggested to use |
vbid = wit.pay_vb; | ||
} | ||
else | ||
FC_THROW("Account ${account} has no core TOKEN vested and thus its not allowed to withdraw.", ("account", witness_name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to throw here shouldn't we do it with an "account is not a witness" message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right but here we are interested in vesting balance only. As per @bobinson 's comment on Jira ticket, We need to throw Assert message if the user doesn't have vesting balance. User can be witness, so we are checking wit.pay_vb
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it.
libraries/wallet/wallet.cpp
Outdated
witness_object wit = get_witness( account_name ); | ||
FC_ASSERT( wit.pay_vb ); | ||
vbid = wit.pay_vb; | ||
if (is_witness(account_name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the withdraw_GPOS_vesting_balance
users will be witnesses. They could be but they dont need to. Assert msg only should change here IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I don't keep this condition then it will fail with "No account or witness named" which doesn't sound relevant for this operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failure you see is in get_witness
. We should not call it at all and go only with the account. Witnesses can withdraw from their vesting balance with withdraw_vesting
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, I will remove checking witness vesting from withdraw_GPOS_vesting_balance
. I've also added the same condition in withdraw_vesting
. I will keep it as it is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
withdraw_vesting
is used to withdraw normal vesting only and not the gpos vesting. Witnesses having gpos vesting will have to call withdraw_GPOS_vesting_balance
so I think I will have to keep this part unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that will never happen. In order for the witnesses to have gpos vesting balance he will have to create and deposit by operation. Payments from the network to the witnesses are created automatically by the blockchain and they are of the default type(normal) with a cdd policy. Witness withdraw this by using withdraw_vesting
.
A witness can have gpos vesting balance but this will be a separated balance from the network payment. In order to withdraw from these the witness will use withdraw_GPOS_vesting_balance
.
At the end withdraw_GPOS_vesting_balance
is totally independent from if the user is a witness so the check can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks.
corrected/improved error messages on following cli wallet commands: