-
Notifications
You must be signed in to change notification settings - Fork 30
N-08: constant names are screaming camel case #247
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
Conversation
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThree Solidity contracts in the finance module have their private storage location constants renamed from camelCase to SCREAMING_SNAKE_CASE nomenclature, with corresponding assembly slot references updated accordingly. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/finance/ERC7821WithExecutor.sol(2 hunks)contracts/finance/VestingWalletCliffConfidential.sol(2 hunks)contracts/finance/VestingWalletConfidential.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: slither
- GitHub Check: tests
- GitHub Check: coverage
🔇 Additional comments (3)
contracts/finance/VestingWalletConfidential.sol (1)
139-139: LGTM!Assembly reference correctly updated to use the renamed constant.
contracts/finance/VestingWalletCliffConfidential.sol (1)
70-70: LGTM!Assembly reference correctly updated to use the renamed constant.
contracts/finance/ERC7821WithExecutor.sol (1)
43-43: LGTM!Assembly reference correctly updated to use the renamed constant.
| // solhint-disable-next-line const-name-snakecase | ||
| bytes32 private constant ERC7821WithExecutorStorageLocation = | ||
| bytes32 private constant ERC7821_WITH_EXECUTOR_STORAGE_LOCATION = | ||
| 0x246106ffca67a7d3806ba14f6748826b9c39c9fa594b14f83fe454e8e9d0dc00; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the inappropriate solhint-disable comment.
The constant now follows SCREAMING_SNAKE_CASE naming, which is the expected convention. The solhint-disable-next-line const-name-snakecase comment is no longer needed and should be removed.
Apply this diff:
- // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.ERC7821WithExecutor")) - 1)) & ~bytes32(uint256(0xff))
- // solhint-disable-next-line const-name-snakecase
bytes32 private constant ERC7821_WITH_EXECUTOR_STORAGE_LOCATION =
0x246106ffca67a7d3806ba14f6748826b9c39c9fa594b14f6748826b9c39c9fa594b14f83fe454e8e9d0dc00;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/finance/ERC7821WithExecutor.sol around lines 18 to 20, remove the
unnecessary solhint suppression comment; delete the line "//
solhint-disable-next-line const-name-snakecase" so the constant declaration
remains and follows SCREAMING_SNAKE_CASE without linter override.
| // solhint-disable-next-line const-name-snakecase | ||
| bytes32 private constant VestingWalletCliffStorageLocation = | ||
| bytes32 private constant VESTING_WALLET_CLIFF_STORAGE_LOCATION = | ||
| 0x3c715f77db997bdb68403fafb54820cd57dedce553ed6315028656b0d601c700; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the inappropriate solhint-disable comment.
The constant now follows SCREAMING_SNAKE_CASE naming, which is the expected convention. The solhint-disable-next-line const-name-snakecase comment is no longer needed and should be removed.
Apply this diff:
- // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.VestingWalletCliffConfidential")) - 1)) & ~bytes32(uint256(0xff))
- // solhint-disable-next-line const-name-snakecase
bytes32 private constant VESTING_WALLET_CLIFF_STORAGE_LOCATION =
0x3c715f77db997bdb68403fafb54820cd57dedce553ed6315028656b0d601c700;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/finance/VestingWalletCliffConfidential.sol around lines 19 to 21,
remove the unnecessary solhint-disable-next-line const-name-snakecase comment
that precedes the VESTING_WALLET_CLIFF_STORAGE_LOCATION constant; delete only
that comment line so the constant remains as-is following SCREAMING_SNAKE_CASE
and no linter suppression remains.
| // solhint-disable-next-line const-name-snakecase | ||
| bytes32 private constant VestingWalletStorageLocation = | ||
| bytes32 private constant VESTING_WALLET_STORAGE_LOCATION = | ||
| 0x78ce9ee9eb65fa0cf5bf10e861c3a95cb7c3c713c96ab1e5323a21e846796800; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the inappropriate solhint-disable comment.
The constant now follows SCREAMING_SNAKE_CASE naming, which is the expected convention. The solhint-disable-next-line const-name-snakecase comment is no longer needed and should be removed.
Apply this diff:
- // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.VestingWalletConfidential")) - 1)) & ~bytes32(uint256(0xff))
- // solhint-disable-next-line const-name-snakecase
bytes32 private constant VESTING_WALLET_STORAGE_LOCATION =
0x78ce9ee9eb65fa0cf5bf10e861c3a95cb7c3c713c96ab1e5323a21e846796800;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/finance/VestingWalletConfidential.sol around lines 37 to 39, remove
the now-unnecessary solhint suppression: delete the comment "//
solhint-disable-next-line const-name-snakecase" that precedes the
VESTING_WALLET_STORAGE_LOCATION constant declaration so the file no longer
contains an inappropriate linter-disable for a constant that already follows
SCREAMING_SNAKE_CASE.
* N-08: constant names are screaming camel case * fix lint
* Start release candidate * Release v0.3.0 (rc) (#221) * Release v0.3.0 (rc) * Update changelog --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> * Update `checkOnTransferReceived` doc (#235) * Versioned Docs (#236) * generate versioned docs * publish docs even on pre-release * N-04: remove unused import in `ERC7984Rwa` (#243) * N-01: reset user instead of allowing user in `unblockUser` (#244) * N-05: Named mapping var in `ERC7984ObserverAccess` (#251) * N-05: Named mapping var in `ERC7984ObserverAccess` * Update contracts/token/ERC7984/extensions/ERC7984ObserverAccess.sol Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> --------- Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> * N-08: constant names are screaming camel case (#247) * N-08: constant names are screaming camel case * fix lint * N-02: reorder allowances omnibus (#250) * Support ERC-165 interface detection on ERC-7984 (#246) * Support ERC-165 interface detection on ERC-7984 * update link format * Add ERC7984 impl changeset * Update changeset --------- Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> * M-03: grant allowances to agent in `ERC7984Rwa` (#242) * M-03: grant allowances to agent in `ERC7984Rwa` * up * N-12: update docs in `ERC7984Restricted` (#245) * Upgrade to use fhevm contracts v0.9.0 (#248) * chore: fhevm-v9 * chore: port all tests for fhevm v9 * Merge pull request #1 from OpenZeppelin/chore/update-disclose-flow update disclose flow * Update wrapper contract (#2) * Update wrapper contract * fix tests * fix mock * update docs * add changeset * request id unnecessary * Update contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> * remove unused params * Update test/token/ERC7984/ERC7984.test.ts Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> * `cts` -> `handles` * `cleartext` -> `cleartextAmount` * Update test/token/ERC7984/extensions/ERC7984Wrapper.test.ts Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> * nit --------- Co-authored-by: 0xalexbel <alexandre.belhoste@zama.ai> Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> * N-[9,11]: fix `ERC7984Rwa` docs (#249) * M-11: fix `ERC7984Rwa` docs * add docs * Update contracts/token/ERC7984/extensions/ERC7984Rwa.sol * L-05: Grant allowances in `confidentialAvailable` (#252) * L-05: Grant allowances in `confidentialAvailable` * fix doc * L-01: `tryDecrease` return initialized value if delta is initialized (#241) * L-01: `tryDecrease` return initialized value if delta is initialized * add comment * Add changeset * Upgrade to use fhevm contracts v0.9.1 (#254) * Upgrade to use fhevm contracts v0.9.1 * bump sub package as well * Update `ERC7984Rwa` docs (#255) * Exit pre-release (#258) * Release v0.3.0 (#253) * Release v0.3.0 * Update changelog (#259) * Update changelog * Update CHANGELOG.md Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> --------- Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> * remove duplicate entry --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> Co-authored-by: 0xalexbel <alexandre.belhoste@zama.ai>
Summary by CodeRabbit