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

L2 coverage #401

Closed
wants to merge 42 commits into from
Closed

L2 coverage #401

wants to merge 42 commits into from

Conversation

shazarre
Copy link
Contributor

@shazarre shazarre commented Oct 22, 2024

Description

Based on an internal audit we discovered some commands that were not L2 compatible. In an effort to increase code coverage and discover potential issues many commands were covered by additional L2 tests:

  • account
  • election
  • governance
  • lockedgold
  • releasecelo
  • validator
  • validatorgroup

That ensures that most of the commands are L2-ready.

Other changes

Test utils were adapted to work with new devchain in L2 context (for example epoch related utils).

Tested

Ran tests locally and on CI.

Related issues

Fixes #406

Follow up issues for potentially non-trivial changes:


PR-Codex overview

This PR introduces enhancements for L2 support in the Celo ecosystem, particularly focusing on validator registration and governance functionalities, while deprecating BLS key requirements.

Detailed summary

  • Added registerValidatorNoBls method to ValidatorsWrapper for L2 registration without BLS keys.
  • Updated validator:register command to make BLS keys optional.
  • Modified tests and commands to support L2 functionalities.
  • Updated documentation to reflect changes in validator registration and governance commands.

The following files were skipped due to too many changes: packages/cli/src/commands/releasecelo/show-l2.test.ts, packages/cli/src/commands/releasecelo/transfer-dollars-l2.test.ts, packages/cli/src/commands/governance/dequeue-l2.test.ts, packages/cli/src/commands/governance/test-proposal-l2.test.ts, packages/cli/src/commands/governance/upvote-l2.test.ts, packages/cli/src/commands/election/revoke-l2.test.ts, packages/cli/src/commands/governance/hashhotfix-l2.test.ts, packages/cli/src/commands/releasecelo/set-beneficiary-l2.test.ts, packages/cli/src/commands/election/vote-l2.test.ts, packages/cli/src/commands/validatorgroup/deregister.test.ts, packages/cli/src/commands/governance/show-l2.test.ts, packages/cli/src/commands/election/run-l2.test.ts, packages/cli/src/commands/election/show-l2.test.ts, packages/cli/src/commands/validator/list.test.ts, packages/cli/src/commands/releasecelo/withdraw-l2.test.ts, packages/cli/src/commands/election/current-l2.test.ts, packages/cli/src/commands/election/activate-l2.test.ts, packages/cli/src/commands/governance/execute-l2.test.ts, packages/cli/src/commands/account/authorize-l2.test.ts, packages/cli/src/commands/releasecelo/authorize-l2.test.ts, packages/cli/src/commands/validator/deregister.test.ts, packages/cli/src/commands/releasecelo/admin-revoke-l2.test.ts, docs/sdk/contractkit/classes/wrappers_Validators.ValidatorsWrapper.md, packages/cli/src/commands/governance/propose-l2.test.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

changeset-bot bot commented Oct 22, 2024

⚠️ No Changeset found

Latest commit: 1e04233

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@shazarre shazarre force-pushed the shazarre/L2_coverage branch from 092bd6c to 1d9840e Compare October 23, 2024 15:06
Copy link

codecov bot commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.29%. Comparing base (23d36cc) to head (1e04233).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #401      +/-   ##
==========================================
+ Coverage   68.01%   70.29%   +2.28%     
==========================================
  Files         288      288              
  Lines       10960    10978      +18     
  Branches     1563     1567       +4     
==========================================
+ Hits         7454     7717     +263     
+ Misses       3430     3187     -243     
+ Partials       76       74       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shazarre shazarre marked this pull request as ready for review November 6, 2024 10:28
@shazarre shazarre requested a review from a team as a code owner November 6, 2024 10:28
Copy link
Member

@aaronmgdr aaronmgdr left a comment

Choose a reason for hiding this comment

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

shipshape

@aaronmgdr
Copy link
Member

merged already 76045eb

@aaronmgdr aaronmgdr closed this Nov 6, 2024
@shazarre shazarre deleted the shazarre/L2_coverage branch November 7, 2024 13:53
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.

celocli hand test/audit on alfajores to ensure L2 readiness
2 participants