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

params: fix bor key-value config look-up #1055

Merged

Conversation

yperbasis
Copy link
Contributor

@yperbasis yperbasis commented Oct 23, 2023

Description

This PR fixes 3 issues:

  1. calculateBorConfigHelper, previously used by CalculateBackupMultiplier & CalculatePeriod incorrectly returned the very last entry for block numbers that matched exactly a key in the map. Practically, it only affected Mumbai since BorMainnet has a single entry in the BackupMultiplier & Period maps.
  2. CalculateBurntContract, used for the upcoming Agra fork, would've suffered from the same issue.
  3. Lexicographic instead of numerical ordering, previously used in borKeyValueConfigHelper, can lead to bugs with blocks like 90M vs 100M (90,000,000 < 100,000,000, but lexicographically "90000000" > "100000000")

Changes

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Changes only for a subset of nodes

Breaking changes

Mumbai-only: CalculateBackupMultiplier(25275000) & CalculatePeriod(25275000) now correctly return 5 instead of 2.

Testing

  • I have added unit tests
  • I have added tests to CI
  • I have tested this code manually on local environment
  • I have tested this code manually on remote devnet using express-cli
  • I have tested this code manually on mumbai
  • I have created new e2e tests into express-cli

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Files Coverage Δ
params/config.go 36.56% <68.75%> (+11.24%) ⬆️

... and 42 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@manav2401 manav2401 changed the base branch from master to v1.1.0-beta-candidate October 23, 2023 12:51
@manav2401 manav2401 changed the title Fix Bor key-value config look-up params: fix bor key-value config look-up Oct 23, 2023
@manav2401 manav2401 merged commit c8548c9 into maticnetwork:v1.1.0-beta-candidate Oct 23, 2023
7 of 8 checks passed
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.

4 participants