feat: solidity contract #44
Conversation
WalkthroughThis PR introduces a yield-bearing ERC20 token contract (NobleDollar) extending Hyperlane's HypERC20, with principal-based accounting and index-driven yield accrual. Supporting utilities handle safe integer conversions and index calculations. Comprehensive tests, documentation, project configuration, and a Hyperliquid variant complete the implementation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
contracts/test/NobleDollar.t.sol (1)
33-35: Consider using dynamic addresses instead of hardcoded constantsHardcoded addresses can make tests brittle and less maintainable. Consider using
makeAddr()or deploying mock contracts.-address constant MAILBOX = 0xc005dc82818d67AF737725bD4bf75435d065D239; -address constant USER1 = 0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045; -address constant USER2 = 0xF2f1ACbe0BA726fEE8d75f3E32900526874740BB; +address MAILBOX = makeAddr("mailbox"); +address USER1 = makeAddr("user1"); +address USER2 = makeAddr("user2");contracts/patches/@hyperlane-xyz%2Fcore@8.0.0.patch (1)
1-68: Recommend thorough security audit of this patchThis patch modifies critical security-related functionality in a third-party dependency. The changes to contract validation and ownership initialization could have significant security implications.
Consider:
- Contributing these changes upstream to Hyperlane
- Conducting a thorough security audit of the patch
- Documenting the rationale for each change
- Implementing comprehensive tests for the modified functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contracts/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.gitignore(1 hunks)contracts/foundry.toml(1 hunks)contracts/package.json(1 hunks)contracts/patches/@hyperlane-xyz%2Fcore@8.0.0.patch(1 hunks)contracts/src/NobleDollar.sol(1 hunks)contracts/test/NobleDollar.t.sol(1 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). (1)
- GitHub Check: build
🔇 Additional comments (11)
.gitignore (1)
4-6: LGTM: Standard build artifacts and dependencies excludedThese additions are appropriate for a smart contract project using Foundry and npm dependencies.
contracts/package.json (1)
4-8: Security vulnerabilities check for dependenciesVerified that OpenZeppelin Contracts v5.3.0 and Hyperlane core v8.0.0 have no publicly disclosed security vulnerabilities as of July 2025.
Recommend enabling automated vulnerability alerts (e.g., dependabot, Snyk) to catch future advisories.• File: contracts/package.json –
@openzeppelin/contracts@5.3.0,@openzeppelin/contracts-upgradeable@5.3.0,@hyperlane-xyz/core@8.0.0contracts/foundry.toml (1)
1-6: LGTM: Standard Foundry configuration with explicit remappingsThe configuration follows best practices by disabling auto-detect remappings and using explicit ones for reproducible builds.
contracts/test/NobleDollar.t.sol (2)
109-110: Potential precision loss in principal calculationThe principal calculation after transfer shows
500055747208instead of exact half. Verify this is expected behavior due to yield accrual effects.Should the principal after 500k transfer be exactly 500055747208, or is there a rounding issue?
90-95: No changes needed for yield calculation logicThe
yield()implementation computesexpectedBalance = principal * index / 1e12; yield = expectedBalance > currentBalance ? expectedBalance - currentBalance : 0;With a principal of 1e12 and an index of 1000111506849,
expectedBalance = 1e12 × 1000111506849 / 1e12 = 1000111506849,
so yield = 1000111506849 − 1000000000000 = 111506849, matching the test. All math is correct.contracts/patches/@hyperlane-xyz%2Fcore@8.0.0.patch (3)
55-65: Verify removal of initial token mintingThe removal of
_totalSupplyparameter and_mint(msg.sender, _totalSupply)significantly changes the token initialization behavior. Ensure this aligns with the intended token economics.This change means tokens won't be minted to the deployer during initialization. Verify this is intentional for the NobleDollar use case and doesn't break existing functionality.
35-40: Ownership initialization via__Ownable_init(_owner)is correct and safeThe one-step call to
__Ownable_init(_owner)atomically sets the intended owner, avoids an intermediate ownership window, and uses fewer storage writes. This matches OpenZeppelin’s recommended pattern in v5.x+ and does not introduce any security regressions. No further changes are needed.
25-30: No additional security risk:code.length > 0is functionally equivalent toAddress.isContract
Both approaches perform the same EVM bytecode‐length check and share identical limitations (during construction and post‐selfdestruct). No new vulnerabilities are introduced by replacingAddress.isContractwith a directcode.lengthcheck.Likely an incorrect or invalid review comment.
contracts/src/NobleDollar.sol (3)
68-91: Well-implemented EIP-7201 storage pattern and initialization logic.The namespaced storage layout follows best practices for upgradeable contracts, and the index initialization to 1e12 provides appropriate precision for yield calculations.
93-129: Correct implementation of view functions and yield calculation.The yield calculation properly handles edge cases and follows the documented formula. The arithmetic ordering (multiply before divide) prevents precision loss.
139-148: Clean and secure yield claiming implementation.The function correctly validates available yield before transfer and emits appropriate events for tracking.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
contracts/README.md (2)
1-6: Minor Markdown style/grammar polishSmall nits that improve readability:
-# USDN Solidity Contract - -## Overview - -The `NobleDollar` contract is the **Solidity implementation** … +# USDN Solidity Contract + +## Overview + +The `NobleDollar` contract is the **Solidity implementation** …– Remove the extra blank line after the top-level header, and ensure only one space after punctuation.
Not blocking but worth cleaning up before publishing to docs.
86-93: Security section could reference standard ERC-20 risksYou list rounding protection & transfer restrictions, but common ERC-20 pitfalls such as re-entrancy during
claim()and upgrade-ability considerations (if behind a proxy) are absent. A short note here will help auditors & integrators.No code change required; just add a bullet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
contracts/README.md
[grammar] ~1-~1: Use correct spacing
Context: # USDN Solidity Contract ## Overview The NobleDollar contract is...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~3-~3: Use proper spacing conventions.
Context: # USDN Solidity Contract ## Overview The NobleDollar contract is the **Soli...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~5-~5: There might be a mistake here.
Context: ...ble Dollar to Ethereum Virtual Machine (EVM) compatible chains through Hyperlane. #...
(QB_NEW_EN_OTHER)
[grammar] ~5-~5: Use correct spacing
Context: ...Machine (EVM) compatible chains through Hyperlane. ## Key Features ### Yield-Bearing Token M...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~7-~7: Use correct spacing
Context: ...tible chains through Hyperlane. ## Key Features ### Yield-Bearing Token Mechanics - **Princ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~10-~10: There might be a mistake here.
Context: ...principal amounts separately from token balances - Automatic yield accrual: Yield is...
(QB_NEW_EN_OTHER)
[grammar] ~11-~11: There might be a mistake here.
Context: ...lly to all token holders based on their principal - Claimable yield: Users can claim ...
(QB_NEW_EN_OTHER)
[grammar] ~12-~12: There might be a mistake here.
Context: ...accumulated yield through the claim() function - Index-based calculations: Uses a ...
(QB_NEW_EN_OTHER)
[grammar] ~13-~13: There might be a mistake here.
Context: ... scaled index (1e12) to calculate yield distributions ### Cross-Chain Interoperability - **Hyperl...
(QB_NEW_EN_OTHER)
[grammar] ~16-~16: There might be a mistake here.
Context: ...rom HypERC20 for seamless cross-chain transfers - Cross-chain yield distribution: R...
(QB_NEW_EN_OTHER)
[grammar] ~17-~17: There might be a mistake here.
Context: ...bution**: Receives yield from Noble via Hyperlane - Unified token experience: Maintai...
(QB_NEW_EN_OTHER)
[grammar] ~18-~18: There might be a problem here.
Context: ...nt yield mechanics across all supported chains ## Architecture The contract extends Hyperlane's `HypER...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~22-~22: Use correct spacing
Context: ... Hyperlane's HypERC20 token router to provide: 1. ERC20 functionality with additional y...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~27-~27: Use correct spacing
Context: ...dates** when new yield is received from Noble ## Core Functions ### View Functions - `...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~29-~29: Use correct spacing
Context: ...w yield is received from Noble ## Core Functions ### View Functions - index(): Returns th...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~31-~31: Use correct spacing
Context: ...from Noble ## Core Functions ### View Functions - index(): Returns the current yield index used ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~33-~33: There might be a mistake here.
Context: ...eturns the current yield index used for calculations - totalPrincipal(): Returns the total...
(QB_NEW_EN_OTHER)
[grammar] ~34-~34: There might be a mistake here.
Context: ...s the total principal amount across all holders - principalOf(address): Returns the p...
(QB_NEW_EN_OTHER)
[grammar] ~35-~35: There might be a mistake here.
Context: ...rns the principal amount for a specific account - yield(address): Calculates claimabl...
(QB_NEW_EN_OTHER)
[grammar] ~36-~36: There might be a mistake here.
Context: ...lculates claimable yield for a specific account ### User Functions - claim(): Claims all...
(QB_NEW_EN_OTHER)
[grammar] ~38-~38: There might be a problem here.
Context: ... yield for a specific account ### User Functions - claim(): Claims all available yield for the caller - St...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~40-~40: There might be a mistake here.
Context: ...()`: Claims all available yield for the caller - Standard ERC20 functions (transfer, approve, etc...
(QB_NEW_EN_OTHER)
[grammar] ~41-~41: There might be a mistake here.
Context: ...ansfer, approve, etc.) with yield-aware accounting ## The _update Function The _update f...
(QB_NEW_EN_OTHER)
[grammar] ~43-~43: Use correct spacing
Context: ...ield-aware accounting ## The _update Function The _update function is the core of t...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~45-~45: Use correct spacing
Context: ...o handle principal accounting and yield distribution. mermaid flowchart LR Start(["Start Transfer Logic"]) --> Super["ERC20 Transfer Logic"] Super --> CheckFromThis{"from == contract?"} CheckFromThis -- Yes --> SkipYieldPayout["Skip Principal Update<br><i>yield payout</i>"] SkipYieldPayout --> End1(["End"]) CheckFromThis -- No --> CheckToThis{"to == contract?"} CheckToThis -- Yes --> CheckMint{"from == 0x0?"} CheckMint -- Yes --> UpdateIndex["Update Index<br><i>yield accrual</i>"] UpdateIndex --> End2(["End"]) CheckMint -- No --> Revert["Revert"] CheckToThis -- No --> CalcPrincipal["Calculate Principal<br><i>round up</i>"] CalcPrincipal --> FromNotZero{"from != zero_address?"} FromNotZero -- Yes --> SubFromPrincipal["Subtract from Principal"] FromNotZero -- No --> AddToTotalPrincipal["Add to Total Principal"] SubFromPrincipal --> ToNotZero{"to != zero_address?"} AddToTotalPrincipal --> ToNotZero ToNotZero -- Yes --> CheckMint2{"from == zero_address?"} CheckMint2 -- Yes --> RecalcPrincipal["Recalculate Principal<br><i>round down</i>"] CheckMint2 -- No --> NoOp1["No Change"] RecalcPrincipal --> AddToPrincipal["Add to Principal"] NoOp1 --> AddToPrincipal AddToPrincipal --> End3(["End"]) ToNotZero -- No --> SubFromTotalPrincipal["Subtract from Total Principal"] SubFromTotalPrincipal --> End4(["End"]) ### Function Behavior Breakdown 1. **Yield...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~74-~74: Use correct spacing
Context: ...nd4(["End"]) ``` ### Function Behavior Breakdown 1. Yield Payout (from == contract): Wh...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~76-~76: There might be a mistake here.
Context: ...ield to users, no principal updates are needed 2. Yield Accrual (`to == contract &...
(QB_NEW_EN_OTHER)
[grammar] ~78-~78: There might be a mistake here.
Context: ...tes the global index based on new total supply - Emits IndexUpdated event 3. **In...
(QB_NEW_EN_OTHER)
[grammar] ~79-~79: There might be a mistake here.
Context: ... total supply - Emits IndexUpdated event 3. Invalid Transfer (`to == contrac...
(QB_NEW_EN_OTHER)
[grammar] ~80-~80: There might be a mistake here.
Context: ...ers from sending tokens directly to the contract 4. Regular Operations: For normal t...
(QB_NEW_EN_OTHER)
[grammar] ~82-~82: There might be a mistake here.
Context: ...es principal using ceiling division for accuracy - Updates sender/recipient principal...
(QB_NEW_EN_OTHER)
[grammar] ~83-~83: There might be a mistake here.
Context: ... - Updates sender/recipient principal balances - Maintains total principal accounti...
(QB_NEW_EN_OTHER)
[grammar] ~84-~84: There might be a mistake here.
Context: ...balances - Maintains total principal accounting ## Security Features - **Rounding Protect...
(QB_NEW_EN_OTHER)
[grammar] ~86-~86: Use correct spacing
Context: ...total principal accounting ## Security Features - Rounding Protection: Uses ceiling div...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~88-~88: There might be a mistake here.
Context: ...s ceiling division to prevent principal manipulation - Transfer Restrictions: Prevents u...
(QB_NEW_EN_OTHER)
[grammar] ~89-~89: There might be a mistake here.
Context: ... unauthorized transfers to the contract address - Index Validation: Ensures yield c...
(QB_NEW_EN_OTHER)
[grammar] ~90-~90: There might be a problem here.
Context: ...d calculations remain consistent across operations ## Events - IndexUpdated: Emitted when yield is accrued and the...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~94-~94: There might be a mistake here.
Context: ... when yield is accrued and the index is updated - YieldClaimed: Emitted when a user c...
(QB_NEW_EN_OTHER)
[grammar] ~95-~95: There might be a mistake here.
Context: ...imed`: Emitted when a user claims their yield - Standard ERC20 events for transfers a...
(QB_NEW_EN_OTHER)
[grammar] ~96-~96: There might be a problem here.
Context: ...Standard ERC20 events for transfers and approvals ## Deployment The contract is deployed with: - **Deci...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~103-~103: There might be a problem here.
Context: ... 1e12 (representing 1.0 with 12 decimal places) ## Integration To integrate with the NobleDollar contr...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~107-~107: Use correct spacing
Context: ...tion To integrate with the NobleDollar contract: 1. Standard ERC20 Integration: Use stand...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~109-~109: There might be a mistake here.
Context: ...Use standard ERC20 interfaces for basic operations 2. Yield Monitoring: Listen for `In...
(QB_NEW_EN_OTHER)
[grammar] ~110-~110: There might be a mistake here.
Context: ...or IndexUpdated events to track yield accrual 3. Yield Claims: Call `yield(addres...
(QB_NEW_EN_OTHER)
[grammar] ~111-~111: There might be a mistake here.
Context: ...imable amounts and claim() to collect yield 4. Cross-Chain Operations: Utilize ...
(QB_NEW_EN_OTHER)
[grammar] ~112-~112: There might be a problem here.
Context: ...ize Hyperlane's routing for cross-chain transfers ## License Copyright 2025 NASD Inc. All rights res...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~116-~116: Use correct spacing
Context: ...se Copyright 2025 NASD Inc. All rights reserved. Licensed under the Apache License, Vers...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
⏰ 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). (1)
- GitHub Check: build
| ## License | ||
|
|
||
| Copyright 2025 NASD Inc. All rights reserved. | ||
|
|
||
| Licensed under the Apache License, Version 2.0. |
There was a problem hiding this comment.
Contradictory license statements – pick one licensing model
“All rights reserved” is incompatible with distribution under the Apache 2.0 license.
Either (a) drop the Apache clause and keep proprietary rights, or (b) keep Apache 2.0 and delete the “all rights reserved” sentence (recommended for open-source).
-## License
-
-Copyright 2025 NASD Inc. All rights reserved.
-
-Licensed under the Apache License, Version 2.0.
+## License
+
+Copyright © 2025 NASD Inc.
+
+This project is licensed under the Apache License, Version 2.0 – see the `LICENSE` file for details.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## License | |
| Copyright 2025 NASD Inc. All rights reserved. | |
| Licensed under the Apache License, Version 2.0. | |
| ## License | |
| Copyright © 2025 NASD Inc. | |
| This project is licensed under the Apache License, Version 2.0 – see the `LICENSE` file for details. |
🧰 Tools
🪛 LanguageTool
[grammar] ~116-~116: Use correct spacing
Context: ...se Copyright 2025 NASD Inc. All rights reserved. Licensed under the Apache License, Vers...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🤖 Prompt for AI Agents
In contracts/README.md around lines 114 to 118, the license section contains
contradictory statements by including both "All rights reserved" and the Apache
License 2.0. To fix this, choose one licensing model: either remove the "All
rights reserved" phrase to comply with the Apache 2.0 open-source license or
remove the Apache License statement to maintain proprietary rights. The
recommended approach is to keep the Apache License 2.0 and delete the "All
rights reserved" sentence.
Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>
Co-authored-by: mesozoic-technology <system@mesozoic.technology>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
contracts/src/NobleDollar.sol (2)
18-18: Thanks for pinning the compiler to 0.8.20Aligns with EIP-7201 guidance and prior review feedback; good call.
187-197: Zero-principal yield accrual: make revert reason explicit here (avoid library DivisionByZero)Right now, with totalPrincipal == 0, this path will revert from IndexingMath with DivisionByZero. Consider guarding and reverting with a domain-specific error for clearer UX/logs, consistent with prior review feedback.
Apply this diff:
- if (to == address(this)) { + if (to == address(this)) { uint128 oldIndex = $.index; - - $.index = IndexingMath.getIndexRoundedDown(totalSupply(), $.totalPrincipal); + if ($.totalPrincipal == 0) { + // Prevent yield accrual before any principal exists to avoid ambiguous DivisionByZero. + revert InvalidTransfer(); + } + $.index = IndexingMath.getIndexRoundedDown(totalSupply(), $.totalPrincipal); emit IndexUpdated(oldIndex, $.index, $.totalPrincipal, value); return; }
🧹 Nitpick comments (6)
contracts/utils/UIntMath.sol (1)
21-24: Nit: wording in NatSpec (“casted”)Minor grammar polish for clarity.
Apply this diff to tighten the wording:
- * @notice Casts a uint256 value to a uint112, ensuring that it is less than or equal to the maximum uint112 value. + * @notice Casts a uint256 value to a uint112, ensuring it is <= type(uint112).max. * @param n The value to cast. - * @return The value casted to uint112. + * @return The value cast to uint112.- * @notice Casts a uint256 value to a uint128, ensuring that it is less than or equal to the maximum uint128 value. + * @notice Casts a uint256 value to a uint128, ensuring it is <= type(uint128).max. * @param n The value to cast. - * @return The value casted to uint128. + * @return The value cast to uint128.Also applies to: 31-34
contracts/utils/IndexingMath.sol (3)
46-51: Nit: comment references to non-existent helpersThe NOTE references multiply112By128Down/Up which aren’t present in this library anymore. This can mislead maintainers.
Apply this diff to update the comment:
- // NOTE: While `uint256(presentAmount) * EXP_SCALED_ONE` can technically overflow, these divide/multiply functions are - // only used for the purpose of principal/present amount calculations for continuous indexing, and - // so for an `presentAmount` to be large enough to overflow this, it would have to be a possible result of - // `multiply112By128Down` or `multiply112By128Up`, which would already satisfy - // `uint256(presentAmount) * EXP_SCALED_ONE < type(uint240).max`. + // NOTE: While `presentAmount * EXP_SCALED_ONE` can technically overflow, in this system `presentAmount` + // derives from principal/index-based calculations bounded by 112/128-bit types, keeping the product + // below ~2^240. Under these bounds, the multiplication is safe in practice.
65-70: Nit: drifted comment, same as aboveMirror the wording fix in the rounded-up variant.
Apply this diff:
- // NOTE: While `uint256(presentAmount) * EXP_SCALED_ONE` can technically overflow, these divide/multiply functions are - // only used for the purpose of principal/present amount calculations for continuous indexing, and - // so for an `presentAmount` to be large enough to overflow this, it would have to be a possible result of - // `multiply112By128Down` or `multiply112By128Up`, which would already satisfy - // `uint256(presentAmount) * EXP_SCALED_ONE < type(uint240).max`. + // NOTE: See overflow note in the rounded-down variant; under 112/128-bit bounds this product stays < 2^240.
96-102: Micro-nit: remove redundant qualificationWithin the same library, referencing
EXP_SCALED_ONEwithoutIndexingMath.reads cleaner.Apply this diff:
- return UIntMath.safe128((presentAmount * IndexingMath.EXP_SCALED_ONE) / principal); + return UIntMath.safe128((presentAmount * EXP_SCALED_ONE) / principal);contracts/src/NobleDollar.sol (2)
144-152: Consider nonReentrant on claim() if hooks are invoked in _updateIf HypERC20’s _update triggers hooks that could reenter, wrap claim with nonReentrant. While balance updates happen in super._update first (mitigating double-claim), defensive hardening is low-cost.
Apply this diff to the function:
-function claim() public { +function claim() public nonReentrant { uint256 amount = yield(msg.sender); if (amount == 0) revert NoClaimableYield(); _update(address(this), msg.sender, amount); emit YieldClaimed(msg.sender, amount); }Additionally, add the following (outside the selected lines) if not already present:
// At top of file: import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol"; // Update the contract declaration: contract NobleDollar is HypERC20, ReentrancyGuard { // ... }
176-178: Nit: zero-address no-op branch may be unnecessaryOpenZeppelin-style _update is typically called with exactly one zero address (mint or burn), not both. If this branch is never hit, consider removing to save a few bytes and reduce cognitive load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
contracts/src/NobleDollar.sol(1 hunks)contracts/utils/IndexingMath.sol(1 hunks)contracts/utils/UIntMath.sol(1 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). (2)
- GitHub Check: prepare
- GitHub Check: build
🔇 Additional comments (9)
contracts/utils/UIntMath.sol (3)
25-28: LGTM: safe112() is correct and gas-cheapBounds check then cast is correct; no unnecessary branches.
35-38: LGTM: safe128() mirrors safe112() correctlyConsistent pattern with appropriate error.
1-3: Verify license compatibility across the repoThis file is GPL-3.0, whereas NobleDollar.sol is Apache-2.0. Mixing GPL and Apache may impose GPL obligations downstream. If the intent is permissive licensing, consider aligning licenses or explicitly documenting the compatibility stance.
Would you like me to scan the repo to inventory SPDX headers and propose a unification plan?
contracts/utils/IndexingMath.sol (2)
15-15: LGTM: EXP_SCALED_ONE choice and type1e12 scaling with a small-width type is reasonable for index math; it keeps bounds tight without risking overflow in the provided operations.
30-34: LGTM: getPresentAmountRoundedDown()The unchecked mul/div is safe under your 112/128 constraints; no external dependency; minimal gas.
contracts/src/NobleDollar.sol (4)
126-134: LGTM: yield() accountingUses floor present value vs actual balance; non-negative and resistant to rounding dust.
80-86: LGTM: EIP-7201 storage locationWell-formed slot derivation and tight packing of index/totalPrincipal; mapping keying looks good.
1-17: Verify license alignment with dependenciesThis file is Apache-2.0 while utility libraries in this PR are GPL-3.0. If you intend permissive licensing for the whole artifact, mixing licenses can be problematic. Either align SPDX across the repo or document the rationale.
Do you want me to inventory SPDX headers and propose a consistent set (e.g., all Apache-2.0)?
88-92: Verify HypERC20 constructor and initialize signaturesHypERC20.sol is imported from the external
@hyperlane/tokenpackage, so we need to ensure that the parameters passed inNobleDollar.solmatch the actual implementation:
Confirm in
node_modules/@hyperlane/token/HypERC20.sol(or your locked dependency) that the constructor signature is:constructor(uint8 _decimals, uint32 _localDomain, address _mailbox)and that
6and1correspond to your intended decimals and localDomain.Confirm that the
initializefunction in the same file is declared as:function initialize( string memory name, string memory symbol, address hook_, address ism_, address caller_ ) publicso that
super.initialize("Noble Dollar", "USDN", hook_, ism_, msg.sender)matches exactly.A mismatch here (e.g., different types or parameter order) will lead to a misconfigured token across domains. Please verify these signatures in your installed
@hyperlane/tokenpackage.
Co-authored-by: toninorair <antonina.norair@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
contracts/test/NobleDollar.t.sol (2)
36-54: Past review comment already addressed this concern.The hardcoded address in the proxy initialization (line 43) was previously flagged. Based on the past review comments, this appears to have been addressed or acknowledged.
56-149: Past review comments already addressed these concerns.Previous reviews flagged:
- The hardcoded hex payloads lacking documentation (lines 58-62, 79-83, etc.)
- Missing negative test cases
The negative test cases concern has been addressed with additional test functions (lines 151-519). The hex payload documentation suggestion remains from past reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/README.md(1 hunks)contracts/src/NobleDollar.sol(1 hunks)contracts/test/NobleDollar.t.sol(1 hunks)contracts/utils/UIntMath.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/utils/UIntMath.sol
- contracts/README.md
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: prepare
🔇 Additional comments (16)
contracts/test/NobleDollar.t.sol (8)
151-169: LGTM!Properly validates the
InvalidTransfererror when attempting to transfer tokens to the contract from a non-zero address.
171-192: LGTM!Properly validates the
InvalidTransfererror for thetransferFrompath, ensuring comprehensive coverage of transfer methods.
194-214: LGTM!Correctly validates both scenarios where
NoClaimableYieldshould be thrown: when no yield has accrued and when an account with zero balance attempts to claim.
216-239: Verify expected overflow behavior.Line 238 expects the mint to silently fail (
mintSuccess == false) when attempting to mint beyonduint112.max. Confirm whether the contract is designed to:
- Silently fail on overflow (current expectation), or
- Revert with an explicit error
If explicit reverts are preferred, update the test to use
vm.expectRevertand ensureIndexingMath.getPrincipalAmountRoundedDownor related code properly reverts on overflow.
241-274: LGTM!Correctly validates that when the index doubles due to yield, subsequent mints receive proportionally less principal, demonstrating proper index-based accounting.
276-433: Comprehensive yield claim coverage.These tests effectively validate:
- Single user claiming 100% yield
- Multiple users claiming proportional yields
- Yield claims after transfers with principal adjustments
- Proper use of
assertApproxEqAbsfor rounding tolerance
435-456: LGTM!Properly validates the edge case where yield accrual occurs with zero total principal, confirming the index remains unchanged and supply increases correctly.
458-518: LGTM!Comprehensive test of burning behavior via
transferRemote, validating:
- Correct principal reduction after burn
- Index progression with reduced principal base
- Yield claims with updated principal values
contracts/src/NobleDollar.sol (8)
1-48: LGTM!License, pragma version (0.8.30), and imports are properly configured. The pinned Solidity version meets EIP-7201 requirements.
50-86: LGTM!Error definitions, event declarations, and ERC7201 storage pattern are correctly implemented:
- Storage struct uses appropriately sized types (
uint128for index,uint112for principals)- Storage location calculation follows ERC7201 specification
- Events are properly indexed
88-96: Verify ifinitializeshould bevirtual.The
initializefunction is markedvirtual, allowing derived contracts to override initialization logic. Given the critical nature of index initialization for yield accounting, confirm whether allowing overrides is intentional.If overriding is not needed, consider removing
virtualto prevent unintended modifications to initialization logic.
98-111: LGTM!Simple, correctly implemented view functions for accessing storage values.
113-136: LGTM!Correctly calculates claimable yield as the difference between expected balance (principal × index) and current balance, properly handling the case where current balance exceeds expected balance.
138-155: LGTM - DOS prevention pattern.The
min()operation on line 148 correctly prevents DOS scenarios where multiple users claim yield simultaneously and the contract balance is insufficient for all claims. Partial claims are supported, withyield()automatically adjusting to reflect remaining claimable amounts.
176-225: Well-structured principal accounting logic.The
_updatefunction correctly handles all transfer scenarios with appropriate principal adjustments:
- Yield payouts: no principal updates (line 183)
- Yield accrual: updates index only (lines 192-202), with proper zero-principal guard (line 193)
- Minting: creates new principal (lines 204-211)
- Transfers/burning: adjusts principal with safe underflow protection (lines 213-224)
The rounding strategy consistently favors the protocol (principal rounded down on mint, up on burn/transfer) to prevent insolvency through rounding exploits, as documented in line 174.
213-216: Verified underflow safety for principal deduction
getSafePrincipalAmountRoundedUpclamps the computed amount tofromPrincipal, ensuring no underflow occurs.
contracts/test/NobleDollar.t.sol
Outdated
| assertEq(usdn.balanceOf(USER1), 15e11, "USER1 should have 1.5M balance after claiming yield"); | ||
|
|
||
| // USER2 yield claim should fail | ||
| vm.expectRevert(abi.encode(NobleDollar.NoClaimableYield.selector)); |
There was a problem hiding this comment.
Fix revert expectation encoding.
Use abi.encodeWithSelector instead of abi.encode for consistency with other tests (e.g., lines 165, 188, 204).
Apply this diff:
- vm.expectRevert(abi.encode(NobleDollar.NoClaimableYield.selector));
+ vm.expectRevert(abi.encodeWithSelector(NobleDollar.NoClaimableYield.selector));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vm.expectRevert(abi.encode(NobleDollar.NoClaimableYield.selector)); | |
| vm.expectRevert(abi.encodeWithSelector(NobleDollar.NoClaimableYield.selector)); |
🤖 Prompt for AI Agents
In contracts/test/NobleDollar.t.sol around line 402, the test uses
vm.expectRevert(abi.encode(NobleDollar.NoClaimableYield.selector)); — change
this to use abi.encodeWithSelector(NobleDollar.NoClaimableYield.selector) to
match the encoding style used elsewhere; replace the abi.encode call with
abi.encodeWithSelector so the revert expectation encodes the selector correctly
and consistently with lines 165/188/204.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/test/NobleDollar.t.sol (1)
365-433: Fix revert expectation encoding on line 402.Line 402 uses
abi.encode(NobleDollar.NoClaimableYield.selector)which is inconsistent with the pattern used elsewhere in the test suite (lines 163, 187, 204, 210). For parameterless errors, use the bare selector; for consistency with encoded expectations, useabi.encodeWithSelector.Apply this diff:
- vm.expectRevert(abi.encode(NobleDollar.NoClaimableYield.selector)); + vm.expectRevert(NobleDollar.NoClaimableYield.selector);Or if you prefer explicit encoding:
- vm.expectRevert(abi.encode(NobleDollar.NoClaimableYield.selector)); + vm.expectRevert(abi.encodeWithSelector(NobleDollar.NoClaimableYield.selector));Test logic: The test itself correctly validates that yield attribution persists across transfers and is distributed proportionally based on principal after subsequent yield accruals.
🧹 Nitpick comments (2)
contracts/test/NobleDollar.t.sol (2)
34-52: Use MAILBOX constant consistently.Line 41 uses a hardcoded address
0x9e6B1022bE9BBF5aFd152483DAD9b88911bC8611instead of theMAILBOXconstant defined at line 30. For clarity and maintainability, use the constant unless this address represents a different entity (in which case, define a new named constant).Apply this diff:
abi.encodeWithSelector( NobleDollar.initialize.selector, - address(0x9e6B1022bE9BBF5aFd152483DAD9b88911bC8611), + MAILBOX, address(new NoopIsm()) )If the address is intentionally different, add a comment explaining why.
54-147: Consider extracting a helper for mailbox payloads.The test comprehensively validates the happy path through multiple yield cycles. However, the hardcoded hex payloads (e.g., lines 59, 80, 131) remain opaque. While functional, a helper function to construct these payloads would significantly improve maintainability and readability.
Example helper:
function _buildMintPayload(address recipient, uint256 amount) internal pure returns (bytes memory) { // Construct and return the encoded payload with comments // ... }This would make tests like:
bytes memory mintPayload = _buildMintPayload(USER1, 1e12);Much clearer than raw hex.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contracts/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
contracts/foundry.toml(1 hunks)contracts/package.json(1 hunks)contracts/patches/@hyperlane-xyz%2Fcore@9.0.9.patch(1 hunks)contracts/src/NobleDollar.sol(1 hunks)contracts/test/NobleDollar.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/package.json
- contracts/foundry.toml
⏰ 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). (2)
- GitHub Check: prepare
- GitHub Check: build
🔇 Additional comments (13)
contracts/patches/@hyperlane-xyz%2Fcore@9.0.9.patch (3)
9-30: Address.isContract replacement is acceptable in this context.The past review raised concerns about replacing
Address.isContractwith directcode.lengthchecks. WhileAddress.isContractdoes provide additional safety (clearer intent, handles edge cases), the simplified check is sufficient here because:
- These modifiers (
onlyContract,onlyContractOrNull) are used during initialization and configuration, not during constructor execution- The check
_contract.code.length > 0reliably detects deployed contracts in post-deployment contexts- This pattern is widely used in production contracts
However, ensure that no code paths attempt to validate contracts during their construction phase, where
code.lengthwould incorrectly return 0.
31-40: LGTM: Ownership initialization updated to modern pattern.The change correctly updates to pass
_ownerdirectly to__Ownable_init(_owner)instead of calling_transferOwnership(_owner)separately. This aligns with newer OpenZeppelin patterns and eliminates redundant operations.
43-67: LGTM: Initialize signature simplified appropriately.Removing the
_totalSupplyparameter and initial_mintcall is appropriate for the yield-bearing token architecture introduced in this PR. The NobleDollar contract handles minting through cross-chain messages with index-based accounting, making initial supply minting at initialization inappropriate.This is a breaking API change to HypERC20, but it aligns with the new use case.
contracts/src/NobleDollar.sol (5)
1-85: LGTM: Well-structured contract foundation.The contract header, imports, and storage pattern are well-implemented:
- Solidity version pinned at 0.8.30 as recommended
- EIP-7201 storage location pattern correctly implemented
- Type sizes are appropriate:
uint128for index (scaled 1e12) anduint112for principal/amounts (max ~5.2e33 base units = 5.2e27 tokens with 6 decimals)- Clean separation of concerns with dedicated math libraries
87-95: LGTM: Correct proxy initialization pattern.The constructor and initializer are properly structured:
_disableInitializers()prevents direct initialization of the implementation- Initial index set to
1e12establishes the correct 1:1 starting ratio- Passes appropriate parameters to
HypERC20(6 decimals, domain 1)
97-135: LGTM: Well-documented view functions.The getter functions are straightforward and correct. The
yield()function deserves special mention:
- Comprehensive documentation explaining the calculation and edge cases
- Correct logic:
max(0, expectedBalance - currentBalance)- Uses
getPresentAmountRoundedDownwhich is conservative (favors protocol)- Handles the case where yield has already been claimed
137-154: LGTM: Robust claim implementation with DOS protection.The claim function is well-implemented:
- DOS protection:
min(balanceOf(contract), yield(user))prevents griefing when contract balance is insufficient to cover all owed yield- Clear error handling with
NoClaimableYieldrevert- Correct event emission
- No reentrancy concerns (state changes in
_update, event after)
156-224: LGTM: Comprehensive and correct yield-bearing transfer logic.The
_updateoverride is well-structured and handles all transfer scenarios correctly:Flow correctness:
- Calls
super._updatefirst to handle balance updates- Early returns for no-ops and yield payouts (no principal changes needed)
- Blocks transfers TO the contract from non-zero addresses (prevents accidental loss)
Yield accrual (lines 189-201):
- Correctly checks
totalPrincipal != 0before division (addresses past review concern)- Returns early without updating index when no principal exists (safe handling)
- Uses
getIndexRoundedDownwhich is conservative (favors protocol)Minting (lines 203-210):
- Rounds principal DOWN (protocol receives slightly more present value per principal unit)
Transfers & burns (lines 212-223):
- Rounds principal UP when deducting (prevents users from transferring with insufficient principal)
- Uses
getSafePrincipalAmountRoundedUpwith sender's principal as max boundRounding strategy: Consistently favors the protocol in all scenarios, preventing economic exploits while being fair to users.
contracts/test/NobleDollar.t.sol (5)
149-214: LGTM: Comprehensive revert test coverage.The revert tests correctly validate:
InvalidTransferwhen attempting to transfer or transferFrom to the contract from a non-zero addressNoClaimableYieldwhen claiming with no available yield or zero balanceThe use of
abi.encodeWithSelectoris correct for errors with parameters, and bare.selectorfor parameterless errors.
216-239: LGTM: Proper uint112 boundary testing.This test correctly verifies:
- Minting the maximum uint112 amount succeeds
- Attempting to exceed uint112 max fails gracefully
This validates the contract's principal overflow protection.
241-274: LGTM: Excellent test of index-based minting.This test validates that new deposits after yield accrual correctly receive proportional principal based on the current index:
- After 100% yield, index = 2.0
- New 1M deposit receives 500k principal (1M ÷ 2.0)
This is a critical property of the yield-bearing mechanism.
276-363: LGTM: Thorough yield claim testing.Both claim tests comprehensively validate:
test_claimYield:
- Correct yield calculation (100% yield scenario)
- Event emission with correct parameters
- Post-claim state: balance doubles, contract balance zeroes, principal unchanged, yield resets to 0
test_claimYieldMultipleUsers:
- Proportional yield distribution (1M and 2M deposits each receive 100% yield)
- Independent claims by multiple users
- Correct state for both users after claiming
435-518: LGTM: Excellent edge case coverage.test_indexUpdateWithZeroTotalPrincipal (lines 435-456):
Validates the fix from past reviews—whentotalPrincipal=0, yield accrual:
- Succeeds without reverting (no division by zero)
- Leaves index unchanged at 1e12
- Increases supply by yield amount
This confirms the guard at line 192 of NobleDollar.sol works correctly.test_indexUpdateAfterBurn (lines 458-518):
Complex scenario testing burn mechanics and subsequent yield:
- After burn, principal is reduced appropriately
- Remaining principal can still claim yield
- Subsequent yield accruals calculate correct index (line 517: index=4e12 is correct for 2M supply on 500k principal)
These tests provide strong coverage of edge cases and the yield/principal accounting system.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
contracts/test/NobleDollar.t.sol (2)
34-54: Hardcoded initialization address still unexplainedThe proxy initialization uses a hardcoded address
0x9e6B1022bE9BBF5aFd152483DAD9b88911bC8611without explanation. This was previously flagged and should either use theMAILBOXconstant or be extracted into a well-named constant with documentation.
404-404: Inconsistent revert encodingLine 404 uses
abi.encode(NobleDollar.NoClaimableYield.selector)instead ofabi.encodeWithSelector(NobleDollar.NoClaimableYield.selector), inconsistent with lines 206, 212 in the same file.contracts/package.json (1)
10-12: Patched dependency concerns remain valid for version 9.0.15The patch applied to
@hyperlane-xyz/core@9.0.15introduces the same critical modifications previously flagged for version 8.0.0, including theAddress.isContractreplacement, altered ownership initialization, and removed initial mint. These changes impact security, initialization behavior, and token supply logic.contracts/patches/@hyperlane-xyz%2Fcore@9.0.15.patch (3)
15-21: Address.isContract replacement still presentThe patch replaces OpenZeppelin's
Address.isContract(_contract)with_contract.code.length > 0, which was previously flagged as a security concern. While functionally similar in Solidity ≥0.8.0,Address.isContractmay include additional validations.
31-40: Ownership initialization pattern changeThe patch modifies the initialization to call
__Ownable_init(_owner)directly instead of__Ownable_init()followed by_transferOwnership(_owner). This assumes an overload exists inOwnableUpgradeable, which may not be standard.
47-66: Initial mint removed from HypERC20 initializationThe patch removes the
_totalSupplyparameter and the_mint(msg.sender, _totalSupply)call from theinitializefunction, fundamentally changing the token initialization behavior. No initial supply is minted, requiring all tokens to come from subsequent operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contracts/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
contracts/foundry.toml(1 hunks)contracts/package.json(1 hunks)contracts/patches/@hyperlane-xyz%2Fcore@9.0.15.patch(1 hunks)contracts/src/HyperliquidNobleDollar.sol(1 hunks)contracts/src/NobleDollar.sol(1 hunks)contracts/test/NobleDollar.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/foundry.toml
⏰ 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). (1)
- GitHub Check: test (TestIBCYieldDistributionTimeout)
| /// @notice Claims all available yield for the bridge and transfers it to the owner. | ||
| function claimForBridge() public { | ||
| uint256 amount = claim(bridge); | ||
|
|
||
| _update(bridge, owner(), amount); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Missing access control on claimForBridge()
The claimForBridge() function is public with no access control, allowing anyone to trigger yield claims for the bridge and transfer those funds to the owner. This could be exploited to grief the bridge, manipulate yield timing, or interfere with bridge operations.
Apply this diff to restrict access:
- function claimForBridge() public {
+ function claimForBridge() public onlyOwner {
uint256 amount = claim(bridge);
_update(bridge, owner(), amount);
}Alternatively, consider whether this function should exist at all, or if the bridge should manage its own yield claims.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @notice Claims all available yield for the bridge and transfers it to the owner. | |
| function claimForBridge() public { | |
| uint256 amount = claim(bridge); | |
| _update(bridge, owner(), amount); | |
| } | |
| } | |
| /// @notice Claims all available yield for the bridge and transfers it to the owner. | |
| function claimForBridge() public onlyOwner { | |
| uint256 amount = claim(bridge); | |
| _update(bridge, owner(), amount); | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/src/HyperliquidNobleDollar.sol around lines 42 to 48, the public
function claimForBridge() lacks access control and allows anyone to trigger
claims that transfer yield to the owner; restrict this by making the function
callable only by an authorized actor (e.g., onlyOwner or onlyBridge): add or
reuse an access-control modifier (import/inherit Ownable if not present or add a
bridge-only modifier), change the function visibility if needed (public ->
external) and apply the modifier to the function declaration, or remove the
function entirely if bridge should manage its own claims; ensure tests and
documentation are updated accordingly.
There was a problem hiding this comment.
One critical issue:
The bridge address is currently set in the constructor of HyperliquidNobleDollar.sol, but this contract is designed to be upgradable (deployed behind a proxy), leading to this value being always address(0) when accessed via the Proxy.
So we have two options:
-
move the initialization to
HyperliquidNobleDollar.soland set the bridge address there. -
If we are 100% that the tokenId will never change, we can make the bridge address immutable and calculate it like now in the constructor. This works because immutable variables are part of the deployed bytecode
* apply fixes to make bridge upgradeable * typo * john's comments * use vm.addr instead
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
contracts/src/HyperliquidNobleDollar.sol (1)
46-51: PermissionlessclaimForBridgeseems intentional; confirm this matches protocol expectations
claimForBridge()is public and lets any caller:
- Claim yield for
bridgevia the internalclaim(bridge), and- Transfer that claimed amount from
bridgetoowner()using_update.Tests explicitly assert that “anyone can claim on behalf of the bridge” and that the yield always flows to the owner, so there’s no direct value-stealing risk. The main effect is that third parties can control when bridge yield is realized and, by design, reallocate part of the bridge principal to the owner.
If that’s the intended economics, the implementation is fine; otherwise, consider tightening this to
onlyOwneror a dedicated bridge role.contracts/test/NobleDollar.t.sol (2)
49-64: Clarify the magic hook address used in proxy initialization
initDatahardcodes0x9e6B1022bE9BBF5aFd152483DAD9b88911bC8611as the hook argument toinitialize. If this is intended to represent a specific mailbox/hook on mainnet, consider extracting it into a named constant (e.g.,HYPERCORE_HOOK) or at least adding a short comment to describe what it is. That will make the test setup easier to understand and maintain alongsideMAILBOXandTOKENID.
424-426: Fix revert expectation encoding forNoClaimableYieldHere you use
vm.expectRevert(abi.encode(BaseNobleDollar.NoClaimableYield.selector));, which produces ABI-encoded data for abytes4value (32 bytes), while the contract reverts with just the 4‑byte selector forNoClaimableYield(). This mismatch will cause the test to fail or not actually verify the revert payload.Align this with your other tests by either:
- vm.expectRevert(abi.encode(BaseNobleDollar.NoClaimableYield.selector)); + vm.expectRevert(BaseNobleDollar.NoClaimableYield.selector);or:
- vm.expectRevert(abi.encode(BaseNobleDollar.NoClaimableYield.selector)); + vm.expectRevert(abi.encodeWithSelector(BaseNobleDollar.NoClaimableYield.selector));Both are correct; the first is shorter and matches the pattern you already use in
test_noClaimableYield.
🧹 Nitpick comments (1)
contracts/src/NobleDollar.sol (1)
88-101: Initialize / UUPS pattern looks sound; consider marking the override asinitializerfor clarityThe constructor disables initializers and the external
initializedelegates tosuper.initialize, which should already be guarded by OZ-styleinitializer, so re-inits will revert. From a readability standpoint, you might still want to declare the override asinitializer(orreinitializerif that’s how Hyperlane exposes it) to make the upgradeable intent explicit at this level, but functionally this is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contracts/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
contracts/src/HyperliquidNobleDollar.sol(1 hunks)contracts/src/NobleDollar.sol(1 hunks)contracts/test/NobleDollar.t.sol(1 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). (2)
- GitHub Check: build
- GitHub Check: prepare
🔇 Additional comments (6)
contracts/src/NobleDollar.sol (3)
133-141: Yield calculation is consistent with index-based accounting
yield(account)usinggetPresentAmountRoundedDown(principal, index)minusbalanceOf(account)with a floor at zero matches the principal/index model and ensures you never report negative yield when users hold more than their “indexed” balance. This aligns with the way_updatekeeps principal in sync, so the view logic here looks correct.
158-169: Claim logic correctly caps by contract balance and avoids principal mutationUsing
UIntMath.min256(balanceOf(address(this)), yield(user))ensures you never overdraw the contract and that partial claims behave gracefully. Delegating the transfer to_update(address(this), user, amount)and relying on thefrom == address(this)early-return to skip principal updates preserves the accounting invariants. The customNoClaimableYielderror covers both “no yield” and “insufficient contract balance” in a single revert, which is a reasonable trade-off.
190-239: _update() branches cover all transfer modes and maintain principal/index invariantsThe override correctly:
- Calls
super._updatefirst to keep ERC20/bridge behavior intact.- Treats
from == address(this)as a pure payout path (no principal changes).- Disallows arbitrary transfers to the contract while allowing mint-to-contract for yield accrual.
- Updates the index only on
to == address(this)mints, with a guard fortotalPrincipal == 0to avoid division by zero.- Mints principal on deposits (
from == address(0)andto != address(this)) and uses a safe rounded-up principal burn/transfer path that’s capped byfromPrincipal.Given the tests you’ve added around burns, second deposits, and multi-user yield, this looks coherent and robust.
If you haven’t already, it’s worth double-checking that
IndexingMath.getSafePrincipalAmountRoundedUpalways returns a value<= fromPrincipalin all edge cases (especially nearuint112limits), since_updaterelies on that guarantee to avoid underflow. A quick targeted test or invariant check around that helper would lock this in.contracts/src/HyperliquidNobleDollar.sol (1)
33-44: Bridge address derivation and initialize flow look consistent
initialize(hook_, ism_, tokenId_)delegating tosuper.initialize(hook_, ism_)and then derivingbridgevia0x20 << 152 | tokenId_aligns with the documented Hyperliquid pattern you’ve quoted, and the test inNobleDollar.t.solassertingbridge()against a concrete address gives good coverage. No changes needed here.contracts/test/NobleDollar.t.sol (2)
216-236:NoClaimableYieldrevert expectations are correct and consistent hereUsing
vm.expectRevert(BaseNobleDollar.NoClaimableYield.selector);leverages thebytes4overload, which matches the revert data for a selector-only custom error and is consistent with the behavior ofclaim()when no yield is available. These negative-path checks for “no yield on fresh mint” and “no yield for zero-balance account” look good.
589-613: Upgrade test covers key invariants across implementations
test_Upgradeexercises:
- Rejection of upgrades from a non-owner.
- Successful upgrade from
owner.- Preservation of critical state (
bridge,index,totalSupply,totalPrincipal).- Ability to use newly-added state (
bridge2) post-upgrade.This is a solid sanity check for the UUPS flow and the EIP‑7201 storage layout assumptions.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.