feat: implement Bill of Exchange functionality in TitleEscrow#66
Open
manishdex25 wants to merge 7 commits into
Open
feat: implement Bill of Exchange functionality in TitleEscrow#66manishdex25 wants to merge 7 commits into
manishdex25 wants to merge 7 commits into
Conversation
- Added status management for TitleEscrow with enum for Issued, Accepted, Rejected, and Discharged. - Implemented functions to accept, reject, and discharge bills of exchange, updating the status accordingly. - Introduced relevant events for state changes and added error handling for invalid status transitions. - Updated Hardhat configuration to optimize contract size for TitleEscrowSignable.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a Bill of Exchange (eBOE) status lifecycle to TitleEscrow contracts: a new ChangesBill of Exchange status lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Holder
participant Beneficiary
participant TitleEscrow
participant TitleEscrowErrors
Holder->>TitleEscrow: acceptBillOfExchange(remark)
TitleEscrow->>TitleEscrow: check status == Issued, beneficiary != holder
alt invalid status
TitleEscrow->>TitleEscrowErrors: revert InvalidBillOfExchangeStatus
else valid
TitleEscrow->>TitleEscrow: status = Accepted, remark updated
TitleEscrow-->>Holder: emit BillOfExchangeAccepted
end
Beneficiary->>TitleEscrow: dischargeBillOfExchange(remark)
TitleEscrow->>TitleEscrow: check status == Accepted
alt invalid status
TitleEscrow->>TitleEscrowErrors: revert InvalidBillOfExchangeStatus
else valid
TitleEscrow->>TitleEscrow: status = Discharged, remark updated
TitleEscrow-->>Beneficiary: emit BillOfExchangeDischarged
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (3)
contracts/TitleEscrow.sol (2)
380-428: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix NatSpec doc:
dischargeBillOfExchangeis caller-restricted to beneficiary, not "Owner".Line 420 says "Owner discharges the instrument" but the function uses
onlyBeneficiary. This is misleading — in this contract, "owner" is ambiguous and could be confused with holder. Update to "Beneficiary" for precision.📝 Proposed doc fix
- /// `@notice` Owner discharges the instrument, moving status from Accepted to Discharged. + /// `@notice` Beneficiary discharges the instrument, moving status from Accepted to Discharged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/TitleEscrow.sol` around lines 380 - 428, Update the NatSpec for dischargeBillOfExchange so it matches the actual access control: the function is gated by onlyBeneficiary, not an owner role. In TitleEscrow’s dischargeBillOfExchange docstring, replace “Owner discharges the instrument” with wording that clearly says the Beneficiary discharges it, keeping the rest of the status-transition description unchanged.
387-392: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
bytes memoryfor the internal helper's remark parameter.
_transitionBillOfExchangeStatusdeclares_remarkasbytes calldata. This works for the three current external callers that pass theircalldataparameter directly, but an internal function withcalldatacannot acceptmemorybytes from future internal callers. Since the function only stores the remark (noabi.decodeor slicing that benefits fromcalldata), usingbytes memorywould be more flexible with negligible gas difference for an internal helper.♻️ Proposed change
- function _transitionBillOfExchangeStatus(Status requiredStatus, Status newStatus, bytes calldata _remark) internal { + function _transitionBillOfExchangeStatus(Status requiredStatus, Status newStatus, bytes memory _remark) internal {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/TitleEscrow.sol` around lines 387 - 392, _update the internal helper _transitionBillOfExchangeStatus to take remark as bytes memory instead of bytes calldata. The current calldata parameter only works for the existing external paths and blocks future internal callers that pass memory bytes; since the helper only assigns to remark and does no calldata-specific processing, switch the parameter type and keep the status/beneficiary checks and assignment logic unchanged._contracts/interfaces/ITitleEscrow.sol (1)
12-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
statusgetter and BOE transition functions to theITitleEscrowinterface.The contract
TitleEscrowimplementsITitleEscrow, but the new publicstatusfield and the three lifecycle functions (acceptBillOfExchange,rejectBillOfExchange,dischargeBillOfExchange) are not declared in the interface. Consumers interacting throughITitleEscrowcannot call these functions or read the status. The tests accessstatusand the BOE functions directly on the contract instance, but any caller using the interface (e.g., viasupportsInterfaceor typed interactions) would be unable to invoke them.♻️ Proposed interface additions
function shred(bytes calldata remark) external; + + function status() external view returns (Status); + + function acceptBillOfExchange(bytes calldata remark) external; + + function rejectBillOfExchange(bytes calldata remark) external; + + function dischargeBillOfExchange(bytes calldata remark) external; }Also applies to: 75-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/interfaces/ITitleEscrow.sol` around lines 12 - 20, The ITitleEscrow interface is missing the public status getter and the BOE lifecycle methods, so add declarations for status plus acceptBillOfExchange, rejectBillOfExchange, and dischargeBillOfExchange to match TitleEscrow’s implemented API. Update the interface alongside the Status enum so typed callers and interface-based interactions can read the lifecycle state and invoke the transition functions through ITitleEscrow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/BillOfExchange.test.ts`:
- Line 2: Prettier formatting is failing in BillOfExchange.test.ts, including
the import statement and several expect(...).to.be.revertedWithCustomError(...)
assertions. Reformat the affected test file to match the project’s Prettier
config, preferably by running the suggested prettier write command so the import
and all listed assertion blocks in BillOfExchange.test.ts are normalized
together.
- Line 885: The typed-data signing in BillOfExchange.test uses
signableEscrow.target for TypedDataDomain.verifyingContract, but that value can
be string | Addressable while the domain requires a string. Update the
endorsement signing setup in the relevant test to pass a string address from
signableEscrow, using the contract’s address accessor (or another string value)
before calling users.holder.signTypedData.
---
Nitpick comments:
In `@contracts/interfaces/ITitleEscrow.sol`:
- Around line 12-20: The ITitleEscrow interface is missing the public status
getter and the BOE lifecycle methods, so add declarations for status plus
acceptBillOfExchange, rejectBillOfExchange, and dischargeBillOfExchange to match
TitleEscrow’s implemented API. Update the interface alongside the Status enum so
typed callers and interface-based interactions can read the lifecycle state and
invoke the transition functions through ITitleEscrow.
In `@contracts/TitleEscrow.sol`:
- Around line 380-428: Update the NatSpec for dischargeBillOfExchange so it
matches the actual access control: the function is gated by onlyBeneficiary, not
an owner role. In TitleEscrow’s dischargeBillOfExchange docstring, replace
“Owner discharges the instrument” with wording that clearly says the Beneficiary
discharges it, keeping the rest of the status-transition description unchanged.
- Around line 387-392: _update the internal helper
_transitionBillOfExchangeStatus to take remark as bytes memory instead of bytes
calldata. The current calldata parameter only works for the existing external
paths and blocks future internal callers that pass memory bytes; since the
helper only assigns to remark and does no calldata-specific processing, switch
the parameter type and keep the status/beneficiary checks and assignment logic
unchanged._
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dce6a803-c7ef-43c9-b3e4-b9977b4d9aed
📒 Files selected for processing (5)
contracts/TitleEscrow.solcontracts/interfaces/ITitleEscrow.solcontracts/interfaces/TitleEscrowErrors.solhardhat.config.tstest/BillOfExchange.test.ts
…ency for verifyingContract - Removed the mintRejected function from the BillOfExchange tests as it was redundant. - Updated the type assertion for verifyingContract to ensure it is treated as a string.
- Reformatted import statements for better organization. - Adjusted expect statements for consistency and clarity in error handling. - Ensured uniformity in the structure of test cases for better maintainability.
isaackps
reviewed
Jul 10, 2026
- Reformatted import statements for better organization. - Adjusted expect statements for consistency and clarity in error handling. - Ensured uniformity in the structure of test cases for better maintainability.
…/token-registry into feature/eboe-bill-of-exchange
- Changed the status variable to override in TitleEscrow. - Updated accept, reject, and discharge functions to override their virtual implementations. - Added status function to contract interfaces for better integration.
- Refactored expect statements for consistency and clarity in error handling. - Enhanced formatting for better organization and maintainability of test cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What is the background of this pull request?
Changes
Issues
What are the related issues or stories?
Releases
Channels: latest
ETA: Any target release date
Summary by CodeRabbit
New Features
statusaccessor.accept,reject, anddischargeactions with role-based permissions, state transitions, and corresponding events (including remark support).Bug Fixes
Tests