fix(pallet): rework billing loop #702
Conversation
renauter
requested review from
DylanVerstraete,
brandonpille,
robvanmieghem and
LeeSmet
as code owners
| @@ -534,8 +534,10 @@ pub mod pallet { | |||
| let _account_id = ensure_signed(origin)?; | |||
|
|
|||
| // Clean up contract from billing loop if it doesn't exist anymore | |||
| // This is the only place it should be done to guaranty we | |||
| continue; | ||
| } | ||
| } | ||
| } | ||
| let _res = Self::bill_contract_using_signed_transaction(contract_id); |
There was a problem hiding this comment.
move this one line up? If there is no contract (deleted somehow but still in billing loop) we don't want to push an extrinsic?
There was a problem hiding this comment.
Hmm, lets check #703 (comment) for this
There was a problem hiding this comment.
move this one line up? If there is no contract (deleted somehow but still in billing loop) we don't want to push an extrinsic?
cannot move this line up because when contract is deleted its id in billing loop is cleaned up at next cycle by pushing the extrinsic
There was a problem hiding this comment.
Yes I just noticed that as well this morning, but it's very weird and hidden behaviour. It's not clear that it's subject to the offchain worker to be cleaned up or not.
There was a problem hiding this comment.
For now let's keep this behaviour but let's plan a future improvement where we redesign the connection between a contract and it's billing status / cycle
| let bill_cu_su = NodeContractResources::<T>::contains_key(contract_id) | ||
| && !NodeContractResources::<T>::get(contract_id).used.is_empty(); |
There was a problem hiding this comment.
why contains_key and get afterwards ? I think get suffices because of the ValueQuery storage definition (default when not present)
There was a problem hiding this comment.
Indeed
But I have a conceptual case here
Ok, I am aware the get will return default value
And what if a default value was intentionally pushed? how to differentiate both scenarios? (nothing pushed / default value pushed)
There was a problem hiding this comment.
And what if a default value was intentionally pushed?
This is something that is bad practice and should be avoided, it also results in the same. To me, this looks like 2 storage reads while it can be only 1 storage read.
There was a problem hiding this comment.
Exactly, in our case it results the same
But if we change the default value for the resource we are broken
So, I ll get rid of extra storage read and add comment
There was a problem hiding this comment.
Why are we broken if we change the default value for resources? .used.is_empty() should always check if all values are 0 and this should not change
There was a problem hiding this comment.
if for some reason we set this for example
pub fn default() -> Resources {
Resources {
hru: 1,
sru: 1,
cru: 1,
mru: 1,
}
}
pub fn empty() -> Resources {
Resources {
hru: 0,
sru: 0,
cru: 0,
mru: 0,
}
}
There was a problem hiding this comment.
well, if the default is any other than 0 values, the user has to pay anyway
| ContractBillingInformationByID::<T>::contains_key(contract_id) | ||
| && ContractBillingInformationByID::<T>::get(contract_id) |
There was a problem hiding this comment.
why contains_key and get afterwards ? I think get suffices because of the ValueQuery storage definition (default when not present)
Related to: