Skip to content
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

Template-Removal v2: refactor test flow #39

Open
wants to merge 4 commits into
base: branch_v.1.1.2r
Choose a base branch
from

Conversation

giladHaimov
Copy link

@giladHaimov giladHaimov commented Dec 21, 2023

This PR introduces changes into the original template-removal pr : #38 , so to omit all test code from system and at the same time introduce a new test-only class - BaseMock - which internally resolves address injection and, being test-only, does not require any special test-mode validations.

this answers for the need for no test code in the system contract. but it does so at a cost:

first: henceforth only mock classes will be testable since non-mock will have no address resolving. before this I have tried to have my tests use the actual contract code which is considered more valid test (and Foundry has a lot of special features to allow just that). I consider the new behavior to be suboptimal, but we'll manage

second: I had to override all address-getter functions in each of the mock classes. the reason for that or more accurately for the reason why getter overriding was not resolved in the BaseMock, was that since the mock class already inherits system via the mock-ed class, it cannot inherit it again via BaseMock, hence have to override each time anew. again - suboptimal but I did not find a way around and frankly do not consider it a deal breaker.

the extended non-sequential base storage module was kept so to allow uint256 reentrancy guard which is way more efficient than boolean.to mask away test-related logic

[Below description of the original template-removal PR as a reference]

Motivation

This PR solves two interconnected problems in our platform-contracts:

Removal of template pre-compilation stage from the contract process
Creation of base-contract extended storage that will allow us (now and in the future) the addition of common-logic behaviors that rely on collisions-safe storage
Since template removal requires production contracts to also accommodate test-mode addresses, the first problem cannot be solved without providing a solution for the second.

Implementation

A. Template Removal

The main motivation our usage of template files was to allow the same code-base for both test and production versions where the main difference between the two was the platform contract addresses that, in the test versions, were dynamically assigned.

To remove the template mechanism while still allowing for a single code base, this PR adds the following mechanisms:

Extended base class storage for contact addresses (plus additional data) that is collision-safe. See details below

An updateContractAddr() function that is now part of the production code but requires constructor invocation (which is guaranteed never to occur on mainnet) for it to become accessible. Now placing updateContractAddr() in the production contract is highly risky. To fire-proof it I use four layers of protection:
a. Constructor must be invoked (i.e. not mainnet) else deployer = zero [will never happen in production]
b. Can only be called by the contract-deployer else onlyTestModeDeployer will revert [will never happen in production]
c. Can only be invoked on non-mainnet else onlyLocalTestMode will fail [will never happen in production]
d. Can never be called more than once [provide a necessary 'trustless' guarantee for platform-address identity]

A set of internal virtual functions that allow the setting of debug values and behaviors by derived mock contracts, thus replacing, in most cases, the need for differential initialization values set by the template files

In several cases the template behavior added, in the test mode, a debug flag to control whether test- or production- logic should take place. My solution here was to duplicate the flag and the differential logic in the mock contracts, so that when the production behavior needed to be called, I simply delegated to the super(=production contract) function, e.g. from CandidateHubMock:

     function _updateRoundTag() internal override {
         if (controlRoundTimeTag) {    // if the controlling flag is on
           roundTag++;   // apply debug behavior
         } else {
           super._updateRoundTag();  // else revert to parent for production behavior
         }
       }
B. Extended base storage

Provide base-contract extended storage that is guaranteed not to result in storage-collusion with the derived concrete contracts, thus allowing for more storage-based common behaviors that we could not add before.

The best explanation I managed to find on how this method of storage allocation works is in this article by the Diamond EIP author.

We have also added ERC-7201 namespace support for the extended storage location, so to reduce the chances of future collision with additional extended storage structures.

The extended base storage is encapsulated in the aptly named ExtStorage structure:

 struct ExtStorage {
    /// @custom:storage-location erc7201:core.system.extended.storage
    Addresses addrs;
    uint256 guardStatus;
    // @dev additional extended-storage fields goes here
  }

Note that any additional state variables added to the System must go to the end of this structure and not to the 'standard' sequential storage

An immediate benefit of using ExtStorage is that it allows us to replace the prior bool reentry guard with a more gas-efficient uint256 guard, same as in Openzeppelin's implementation that we cannot use due to storage-collision considerations.

Finally, this PR adds, as a side-task, additional excessive-token handling logic to the SystemReward's receive() function. Due to this addition, and the gas cost it incurs, SystemReward may no longer receive Core via send() or transfer() (both limited to 2300 gas units). Rather, Core may now only be passed via low-level call{ value}("") or a wrapper like sendValue() where no limit is imposed on gas consumption:

_systemReward.send(val); // bad

_systemReward.transfer(val); // bad

_systemReward.sendValue(val); // good

@giladHaimov giladHaimov changed the title Introducing abstract BaseGenesisContract Template-Removal v2: refactor test flow Dec 24, 2023
@halbornteam
Copy link
Collaborator

It can be added noEmptyDeposit modifier on the deposit function : ValidatorSet.sol#L111

@halbornteam
Copy link
Collaborator

The RLPEncode library's encodeBool function originally encoded boolean values for RLP (Recursive Length Prefix) incorrectly, particularly for false. This implementation only encoded true as 0x01 but did not correctly encode false. In RLP, false should be encoded differently from an empty string or absence of data. Also, The toBoolean function in the RLPDecode library is designed to decode RLP-encoded boolean values. The adjustment in the encoding scheme (0x80 for false) must be accurately reflected in the decoding process to maintain consistency.

Current behaviour

  • 0x80 - decoded as true on smart contract.
  • 0x01 - decoded as true on smart contract.

Expected behaviour

  • 0x80 - decoded as false on smart contract.
  • 0x01 - decoded as true on smart contract.
  • 0x00 - decoded as false on smart contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants