remove epoch related code from light client contract #1934

alysiahuggins · 2024-08-28T18:40:57Z

Closes #1928

This PR:

Simplifies the Light Client contract by removing any functionality related to epochs since epochs have not been implemented yet. Code Changes:

Removes the following from the LightClient contract and related code
- the blocksPerEpoch variable
- removes currentEpoch variable
Removes the following from the StakeTable contract
- currentEpoch()
- removes the tests as the stake table isn't used and the current version heavily depends on epochs which have not been implemented yet
Update the diff-test code

This PR does not:

remove epoch the StakeTable contracts as they are not used, instead the relevant tests are ignored when running just sol-test

Key places to review:

Updated Tests
Implementation Code
Deployment Scripts
Modified Rust Scripts

… to pure

…poch-related-code

philippecamacho · 2024-08-28T23:28:01Z

contracts/src/StakeTable.sol

    /// @dev Fetches the current epoch from the light client contract.
    /// @return current epoch (computed from the current block)
-    function currentEpoch() public view returns (uint64) {
-        return lightClient.currentEpoch();
+    function currentEpoch() public pure returns (uint64) {


Why do we need to keep the function currentEpoch()?

at the time i made the change, i wanted to keep the related tests with as little breaking changes as possible - but now that i removed the stateTable tests, i can remove that function

i also agree to keep this, a dummy logic is perfect here. Plus we are not using or auditing this contract anyway.

philippecamacho · 2024-08-28T23:29:14Z

contracts/test/LightClient.t.sol

@@ -21,7 +21,7 @@ contract LightClientCommonTest is Test {
    uint32 public constant BLOCKS_PER_EPOCH_TEST = 3;
    uint32 public constant DELAY_THRESHOLD = 6;
    uint32 public constant MAX_HISTORY_SECONDS = 1 days;
-    uint32 initialEpoch = 1 days;
+    uint32 public initialEpoch = 1 days;


Why do we need to keep this variable?

ah this epoch isn't used for testing the block per epoch functionality but for testing the retention period for storing the state history on the contract

I see, should we change the name to avoid confusion?

done here 5acfd94

…gly.

alxiong

Thanks for the work! 👍

alxiong · 2024-08-29T03:27:32Z

contracts/src/StakeTable.sol

    /// @dev Fetches the current epoch from the light client contract.
    /// @return current epoch (computed from the current block)
-    function currentEpoch() public view returns (uint64) {
-        return lightClient.currentEpoch();
+    function currentEpoch() public pure returns (uint64) {


i also agree to keep this, a dummy logic is perfect here. Plus we are not using or auditing this contract anyway.

contracts/test/LightClient.t.sol

alxiong · 2024-08-29T03:48:09Z

hotshot-state-prover/src/mock_ledger.rs

+/// Number of blocks per epoch for testing
+pub const BLOCKS_PER_EPOCH: u32 = 3;
+


can we set to u32::MAX for now?

since the blocks_per_epoch is used to determine how many states to generate, which is then used in the tests, I would say we need to choose a small number. I would opt for 5 though

okay, sounds good.

contracts/test/StakeTable.t.sol

…s from forge test when using just sol-test

alxiong

LGTM, one comment is: instead of forge test --no-match-contract, can we edit foundry.toml to ignore?
Because locally I won't have to remember to add that flag to forge commands all the time.

Thanks!

…poch-related-code

alysiahuggins · 2024-08-29T16:16:29Z

LGTM, one comment is: instead of forge test --no-match-contract, can we edit foundry.toml to ignore? Because locally I won't have to remember to add that flag to forge commands all the time.

Thanks!

@alxiong I agree, however, ignored_warnings_from in the foundry.toml didn't work. foundry-rs/foundry#4414

…st sol-test and proceed with changes to the diff-test

philippecamacho

LGTM

remove blocksPerEpoch from LC

22dcab7

alysiahuggins linked an issue Aug 28, 2024 that may be closed by this pull request

simplify light client contract: remove epoch related code #1928

Closed

alysiahuggins added 3 commits August 28, 2024 18:28

removed currentEpoch varible, function and stake table tests

317dfa4

restricting visibility of the currentEpoch function on the StakeTsble…

8605c19

… to pure

Merge branch 'main' into 1928-simplify-light-client-contract-remove-e…

a1e5e0d

…poch-related-code

alysiahuggins changed the title ~~remove blocksPerEpoch from LC~~ remove epoch related code from light client contract Aug 28, 2024

alysiahuggins requested review from philippecamacho and alxiong August 28, 2024 22:58

alysiahuggins marked this pull request as ready for review August 28, 2024 22:58

alysiahuggins requested review from nomaxg, ImJeremyHe, sveitser, jbearer, tbro and imabdulbasit as code owners August 28, 2024 22:58

re-enable ffi

1a7bb3e

philippecamacho reviewed Aug 28, 2024

View reviewed changes

philippecamacho added 2 commits August 28, 2024 20:17

Make the block_per_epoch argument optional for the mock-genesis command.

384eea8

Remove BLOCKS_PER_EPOCH_TEST and adjust diff-test executable accordin…

e7e7029

…gly.

alxiong reviewed Aug 29, 2024

View reviewed changes

alysiahuggins added 3 commits August 29, 2024 04:46

updated comments to remove mention of epoch

5acfd94

variable rename

2614c8a

removed epoch from deployment scripts and skipped staketable contract…

3f6ecc2

…s from forge test when using just sol-test

alysiahuggins requested review from philippecamacho and alxiong August 29, 2024 16:04

alxiong approved these changes Aug 29, 2024

View reviewed changes

Merge branch 'main' into 1928-simplify-light-client-contract-remove-e…

e666ebf

…poch-related-code

remove the stake table related tests, update forge test command in ju…

0086ab8

…st sol-test and proceed with changes to the diff-test

philippecamacho approved these changes Aug 29, 2024

View reviewed changes

alysiahuggins merged commit 1587f4a into main Aug 29, 2024
13 checks passed

alysiahuggins deleted the 1928-simplify-light-client-contract-remove-epoch-related-code branch August 29, 2024 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove epoch related code from light client contract #1934

remove epoch related code from light client contract #1934

alysiahuggins commented Aug 28, 2024 •

edited

Loading

philippecamacho Aug 28, 2024

alysiahuggins Aug 28, 2024

alxiong Aug 29, 2024

philippecamacho Aug 28, 2024

alysiahuggins Aug 28, 2024

philippecamacho Aug 29, 2024

alysiahuggins Aug 29, 2024

alxiong left a comment •

edited

Loading

alxiong Aug 29, 2024

alxiong Aug 29, 2024

alysiahuggins Aug 29, 2024

alxiong Aug 29, 2024

alxiong left a comment

alysiahuggins commented Aug 29, 2024 •

edited

Loading

philippecamacho left a comment

		/// Number of blocks per epoch for testing
		pub const BLOCKS_PER_EPOCH: u32 = 3;

remove epoch related code from light client contract #1934

remove epoch related code from light client contract #1934

Conversation

alysiahuggins commented Aug 28, 2024 • edited Loading

This PR:

This PR does not:

Key places to review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxiong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxiong left a comment

Choose a reason for hiding this comment

alysiahuggins commented Aug 29, 2024 • edited Loading

philippecamacho left a comment

Choose a reason for hiding this comment

alysiahuggins commented Aug 28, 2024 •

edited

Loading

alxiong left a comment •

edited

Loading

alysiahuggins commented Aug 29, 2024 •

edited

Loading