-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix(contracts): return ok when amount is 0 to distribute #707
Conversation
log::info!( | ||
"twin balance {:?} contract lock amount {:?}", | ||
twin_balance, | ||
contract_lock.amount_locked | ||
); |
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.
Ok, the idea here is, ideally, to make the user pay for amount_locked
and if he has not enough funds to drain its account, correct?
And then reset its amount_locked
to 0
Let s imagine an extreme case where usuable_balance = 1
and amount_locked = 1000
So the user will effectively pay 1
and the remaining 999
due will vanish
Is that what we want?
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.
yes, but this is only a viable flow in the case that a contract is in grace period or a contract got canceled. We cannot take more funds out of the users wallet then whatever he has left.
In the normal billing case, the user will always pay whatever he's due.
); | ||
|
||
// If the amount is zero, return | ||
if amount == BalanceOf::<T>::saturated_from(0 as u128) { |
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 follow the logic above, if amount == 0
it is because user usable_balance == 0
Wouldn't be better if we handle this case at beginning of bill_contract()
Billing a user with no funds doesn't make sense...
Or maybe because bill_contract()
does a lot more than billing the user 🥲
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.
bill_contract
has to many flows to just return at the start of the function.
|
||
// If the amount is zero, return | ||
if amount == BalanceOf::<T>::saturated_from(0 as u128) { | ||
return Ok(().into()); |
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.
Other point
Why do we handle this case inside _distribute_cultivation_rewards()
Could be interesting to be more explicit upward like this?
let billable_amount = twin_balance.min(contract_lock.amount_locked);
if billable_amount != BalanceOf::<T>::saturated_from(0 as u128) {
// Distribute cultivation rewards
match Self::_distribute_cultivation_rewards(
&contract,
&pricing_policy,
billable_amount,
) { ... }
}
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.
because we need to wrap an entire code block in an If-statement, I prefer the cleaner early return in the function.
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.
LGTM
Just added some suggestions / ideas for billing redesign
When amount is 0 to distribute, return Ok so the calling function can continue it's work.