feat: Remove ContractsInfo #673
Conversation
fuel-vm/src/storage.rs
Outdated
| @@ -31,16 +31,31 @@ impl Mappable for ContractsRawCode { | |||
| type Value = [u8]; | |||
| } | |||
|
|
|||
| /// The storage table for contract's additional information as salt, root hash, etc. | |||
| /// The storage table for contract's additional information | |||
| pub struct ContractsInfo; | |||
There was a problem hiding this comment.
I am not even sure that we need this table here=) Since we don't use salt in any logic, we can simply skip it, and it can be fully off-chain information.
|
And we need to update |
bvrooman
changed the title
| croo: DependentCost::LightOperation { | ||
| base: 1, | ||
| units_per_gas: 1, | ||
| }, |
There was a problem hiding this comment.
Updating the costs is captured in a follow up task here: FuelLabs/fuel-core#1660
| ContractId::from_bytes_ref(contract_id.read(&self.memory)); | ||
| let code_size = contract_size(&self.storage, contract_id)? as Word; | ||
| let gas_cost = self.gas_costs().croo.resolve(code_size); | ||
| self.gas_charge(gas_cost)?; |
There was a problem hiding this comment.
We need to charge for it in the code_root. You can see how it was done in the code_size
| @@ -1021,13 +1024,55 @@ fn code_copy_c_gt_vm_max_ram() { | |||
|
|
|||
| #[test] | |||
| fn code_root_a_plus_32_overflow() { | |||
There was a problem hiding this comment.
These tests are pretty noisy, and very imperative. It would be nice to kinda hide the details behind declarative functions deploy_contract, etc. That code can be shared as well.
Also given/when/then would be useful as well.
There was a problem hiding this comment.
This is much better. I'm still kinda confused what these tests are for, but it's not really your job to fix all the tests in this PR.
Ideally, we just have one SUT (system under test) per tests and a single variable. And the name of the tests just is sut__when_x_then_y, but this
check_expected_reason_for_instructions_with_client we are using everywhere is hiding the SUT from us. i.e. we are expecting MemoryOverflow, but I'm not sure what we are even doing that is generating that error.
Kinda a perfect example of how writing code with DRY as a top priority can create a lack of clarity.
There was a problem hiding this comment.
I agree. We lacked a proper standard for testing at the time and the consequence is an unstructured and incongruent series of tests across the codebase. This series of tests specifically could be rewritten to make the SUT, test setup, and success conditions more explicit, but I won't address it all at this time. It ultimately represents technical debt, and hopefully we can address it at some point.
| let mut cgas = 0; | ||
| let mut ggas = 0; |
There was a problem hiding this comment.
It would be nice to check that cgas and ggas are udpated=)
There was a problem hiding this comment.
I refactored the CROO test into croo_tests.rs and broke it up into individual tests. Each test now checks the cgas and ggas explicitly, for both success and failure cases.
Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
Related issues:
ContractsInfotype fuel-core#1654