Charge for the actual size of the contract in ccp opcode
#637
Conversation
| self.gas_charge(gas_cost.base())?; | ||
| gas_cost.set_base(0); |
There was a problem hiding this comment.
I know we already use this pattern where we:
- Charge the base of the dependent cost
- Set the base of the dependent cost to 0
- Charge the variable amount of the dependent cost
In general, I do not think this pattern makes a lot of sense. The structure of DependentCost doesn't line up with how it's used: We basically split the one dependent cost into both a fixed cost and dependent cost so we can charge at different times. We break encapsulation by reading and charging base as a fixed cost, then we mutate base to 0, which means CodeCopyCtx (and other places we do this) requires a nonstandard mutated DependentCost input, but there's no type safety or documentation to let developers know. All of this breaks the principle of least surprise.
Maybe we could break up dependent costs into both a DependentCost and fixed cost, where dependent cost is just the variable amount and the fixed cost is the base. This way, we have type safety and no gymnastics to fit our two-stage charging pattern.
There was a problem hiding this comment.
I tried to minimise the number of changes, but if you want to, its okay to do=) Updated in the 7d9cfdd
| @@ -80,20 +80,27 @@ fn test_load_contract() -> IoResult<(), Infallible> { | |||
| fn test_code_copy() -> IoResult<(), Infallible> { | |||
There was a problem hiding this comment.
Is there any coverage that needs to be added to the blockchain tests? Like we have all these for ldc here. Specifically, it might be good to have a test that covers the case where contract size is larger than $rD and show the cost is proportional to the contract that is being copied rather than the bytes specified.
There was a problem hiding this comment.
But the test already is doing that=)
The test is done in the same way as it was done for test_load_contract(test above)
Co-authored-by: Brandon Vrooman <brandon.vrooman@fuel.sh>
…nto bugfix/charge-ccp-actual-size
Duplicates #598 but for ccp