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

fix(offchain-voting): remove duplicate checks; set node index to start from 1; tests #534

Merged
merged 13 commits into from
Apr 2, 2022

Conversation

fforbeck
Copy link
Contributor

@fforbeck fforbeck commented Mar 23, 2022

Proposed Changes

  • Removed _verifyNode redundant check from OffchainVoting.sol
  • Executing the isReadyToSubmitResult check for every new vote result
  • Removed checkMemberCount from OffchainVotingHelper.sol and OffchainVoting.submitResult because this check is already done via getBadNodeError.INDEX_OUT_OF_BOUNDS.
  • Fixed challengeMissingStep - missing the ACLs to submit the proposals and jail members.
  • Improved revert messages.
  • Refactor to consider the node.index starting from 1 instead of 0.
  • Improved the test coverage for the offchain voting contracts.

@fforbeck fforbeck marked this pull request as ready for review March 23, 2022 20:42
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #534 (fd3285f) into master (2f70d3d) will increase coverage by 5.13%.
The diff coverage is 100.00%.

❗ Current head fd3285f differs from pull request most recent head 0292f7e. Consider uploading reports for the commit 0292f7e to get more accurate results

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   85.59%   90.72%   +5.13%     
==========================================
  Files          52       52              
  Lines        1867     1866       -1     
  Branches      438      438              
==========================================
+ Hits         1598     1693      +95     
+ Misses        269      173      -96     
Impacted Files Coverage Δ
contracts/adapters/voting/OffchainVoting.sol 95.13% <100.00%> (+53.88%) ⬆️
contracts/adapters/voting/OffchainVotingHash.sol 77.27% <100.00%> (+28.43%) ⬆️
contracts/helpers/OffchainVotingHelper.sol 82.53% <100.00%> (+9.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f70d3d...0292f7e. Read the comment docs.

@fforbeck fforbeck changed the title wip: adding tests fix(offchain-voting): ... Mar 24, 2022
@fforbeck fforbeck force-pushed the trb-9 branch 2 times, most recently from 1f08d07 to 3515fe4 Compare March 25, 2022 19:09
@fforbeck fforbeck added the fix important fix label Mar 29, 2022
@fforbeck fforbeck self-assigned this Apr 2, 2022
@fforbeck fforbeck changed the title fix(offchain-voting): ... fix(offchain-voting): remove duplicate checks; set node index to start from 1; tests Apr 2, 2022
@fforbeck fforbeck merged commit f128c1d into master Apr 2, 2022
@fforbeck fforbeck deleted the trb-9 branch April 2, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix important fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant