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: remove use of magic numbers in __ProvisionManager_init_unchained function (OZ N-11) #1041

Merged
merged 8 commits into from
Oct 1, 2024

Conversation

MoonBoi9001
Copy link
Member

Motivation:

Title:

N-11 Use Constants for Default Values

Details:

In the ProvisionManager contract, the __ProvisionManager_init_unchained function initializes multiple default values such as type(uint256).min, type(uint256).max, type(uint32).min, type(uint32).max, type(uint64).min, and type(uint64).max directly within the function body.

This approach introduces magic numbers, reducing code readability and maintainability. Moreover, it could potentially introduce errors since the same literal values are used in another part of the codebase as well. In addition, the verifierCutRange is set between 0 and uint32.max but the maximum should be set to MAX_PPM.

Consider revising all uses of literal values, defining constants for them where applicable, and setting a more sensible maximum value for verifierCutRange.

Key changes

  • Removed the use of magic numbers inside the __ProvisionManager_init_unchained function, now using constants for default values.

@MoonBoi9001 MoonBoi9001 changed the title fix: address and remove the reliance on magic numbers (OZ N-11) fix: remove use of magic numbers in __ProvisionManager_init_unchained function (OZ N-11) Sep 18, 2024
Copy link

openzeppelin-code bot commented Sep 18, 2024

fix: remove use of magic numbers in __ProvisionManager_init_unchained function (OZ N-11)

Generated at commit: ffd1f79c1ec9b397de64be6ef8e7d3714b575ae0

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
4
0
15
40
61
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

Copy link
Contributor

@tmigone tmigone left a comment

Choose a reason for hiding this comment

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

We also need to use these variables in SubgraphService.sol in the following functions:

  • setMinimumProvisionTokens
  • _getThawingPeriodRange
  • _getVerifierCutRange

That contract inherits from ProvisionManager.sol so the variables should be available.

@@ -23,6 +23,14 @@ import { ProvisionManagerV1Storage } from "./ProvisionManagerStorage.sol";
abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionManagerV1Storage {
using UintRange for uint256;

// Constants
uint32 private constant DEFAULT_MIN_UINT32 = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think better names for these would be tied to the range they are describing. For example, instead of DEFAULT_MIN_UINT256 and DEFAULT_MAX_UINT256 you would have DEFAULT_MIN_PROVISION_TOKENS and DEFAULT_MAX_PROVISION_TOKENS and so forth.

uint256 private constant DEFAULT_MIN_UINT256 = 0;
uint64 private constant DEFAULT_MAX_UINT64 = type(uint64).max;
uint256 private constant DEFAULT_MAX_UINT256 = type(uint256).max;
uint32 private constant MAX_PPM = uint32(PPMMath.MAX_PPM);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would use DEFAULT_MAX_VERIFIER_CUT

Copy link
Contributor

@tmigone tmigone left a comment

Choose a reason for hiding this comment

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

We also need to use these variables in SubgraphService.sol in the following functions:

  • setMinimumProvisionTokens
  • _getThawingPeriodRange
  • _getVerifierCutRange

@MoonBoi9001
Copy link
Member Author

I made a few of these internal so they can be inherited.

    // Constants
    uint32 private constant DEFAULT_MIN_VERIFIER_CUT = type(uint32).min;
    uint32 internal constant DEFAULT_MAX_VERIFIER_CUT = uint32(PPMMath.MAX_PPM);
    uint64 private constant DEFAULT_MIN_THAWING_PERIOD = type(uint64).min;
    uint64 internal constant DEFAULT_MAX_THAWING_PERIOD = type(uint64).max;
    uint256 private constant DEFAULT_MIN_PROVISION_TOKENS = type(uint256).min;
    uint256 internal constant DEFAULT_MAX_PROVISION_TOKENS = type(uint256).max;

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.51%. Comparing base (374f57d) to head (f8e39df).
Report is 18 commits behind head on horizon.

Additional details and impacted files
@@           Coverage Diff            @@
##           horizon    #1041   +/-   ##
========================================
  Coverage    92.51%   92.51%           
========================================
  Files           46       46           
  Lines         2392     2392           
  Branches       428      428           
========================================
  Hits          2213     2213           
  Misses         179      179           
Flag Coverage Δ
unittests 92.51% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@tmigone tmigone left a comment

Choose a reason for hiding this comment

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

Couple nits!

Copy link
Contributor

@tmigone tmigone left a comment

Choose a reason for hiding this comment

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

💎

@MoonBoi9001 MoonBoi9001 merged commit 3a51379 into horizon Oct 1, 2024
8 checks passed
@MoonBoi9001 MoonBoi9001 deleted the fix_oz_n-11 branch October 1, 2024 17:02
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.

2 participants