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

feat: EBOFinalityModule #2

Merged
merged 16 commits into from
Aug 13, 2024
Merged

feat: EBOFinalityModule #2

merged 16 commits into from
Aug 13, 2024

Conversation

0xJabberwock
Copy link
Member

🤖 Linear

Closes GRT-14

@0xJabberwock 0xJabberwock self-assigned this Jul 30, 2024
Copy link

linear bot commented Jul 30, 2024

GRT-14 Build EBOFinalityModule

The EBOFinalityModule will receive the response and emit the relevant events and allows an address to push an amendment

design

src/contracts/EBOFinalityModule.sol Outdated Show resolved Hide resolved
src/contracts/EBOFinalityModule.sol Outdated Show resolved Hide resolved
Comment on lines 54 to 59
// uint256 _length = _response.chainIds.length;
// if (_length != _response.blocks.length) revert EBOFinalityModule_LengthMismatch();

// for (uint256 _i; _i < _length; ++_i) {
// emit NewEpoch(_response.epoch, _response.chainIds[_i], _response.blocks[_i]);
// }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Response struct would first need to be redeclared for this to work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, add a todo and just put a decode here as to how the response "could be" and then me modify

Comment on lines 75 to 80
/// @inheritdoc IModule
function validateParameters(bytes calldata _encodedParameters)
external
view
override(Module, IModule)
returns (bool _valid)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will EBOFinalityModule override validateParameters()?

@0xJabberwock 0xJabberwock marked this pull request as ready for review July 31, 2024 06:18
Comment on lines 54 to 59
// uint256 _length = _response.chainIds.length;
// if (_length != _response.blocks.length) revert EBOFinalityModule_LengthMismatch();

// for (uint256 _i; _i < _length; ++_i) {
// emit NewEpoch(_response.epoch, _response.chainIds[_i], _response.blocks[_i]);
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, add a todo and just put a decode here as to how the response "could be" and then me modify

src/interfaces/IEBOFinalityModule.sol Show resolved Hide resolved
_validateResponse(_request, _response);

// uint256 _length = _response.chainIds.length;
// if (_length != _response.blocks.length) revert EBOFinalityModule_LengthMismatch();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have just one chain per response in the end

@0xJabberwock 0xJabberwock marked this pull request as draft August 6, 2024 02:27
@0xJabberwock 0xJabberwock marked this pull request as ready for review August 11, 2024 03:24
Copy link
Contributor

@ashitakah ashitakah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks so good ! Just a few comments!

src/contracts/EBOFinalityModule.sol Show resolved Hide resolved
test/unit/EBOFinalityModule.t.sol Show resolved Hide resolved
}

contract EBOFinalityModule_Unit_FinalizeRequest is EBOFinalityModule_Unit_BaseTest {
struct FinalizeRequestParams {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct doesnt exist in the contract, maybe we shouldn't use it and create the happy path with each param

Copy link
Member Author

@0xJabberwock 0xJabberwock Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment of fuzz testing—specially when there are many parameters to fuzz—I like taking advantage of custom structs because:

  • Their members can be adjusted out of the scope of the test case function (e.g., within the happyPath modifier) due to its variable being a memory pointer.
  • They work around stack too deep errors when the amount of parameters is large.
  • They concentrate all the fuzzed parameters in one place, easing the burden of eventual maintenance.
  • They minimize the visual load and improve readability of test case functions.

@ashitakah ashitakah merged commit 640568e into dev Aug 13, 2024
7 checks passed
@ashitakah ashitakah deleted the feat/ebo-finality-module branch August 13, 2024 19:33
0xShaito pushed a commit that referenced this pull request Aug 14, 2024
0xShaito pushed a commit that referenced this pull request Aug 14, 2024
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.

3 participants