Use Max rate to charge for Notice Period #95
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances smart contract functionality and its associated tests by introducing a new field (maxRate) in the MarketV1 contract to accurately track rate changes, updates event definitions to include timestamp data, and improves deposit/withdrawal as well as emergency procedures. Key changes include:
- Updates in MarketV1.sol to remove the extra settleTill parameter and compute settlement using block.timestamp, while integrating maxRate for rate revisions.
- Enhancements in Credit.sol with pause/unpause capabilities and refined redeemAndBurn functionality.
- Adjustments in tests and deployment scripts to align with the new contract logic and behavior.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/token/Credit.ts | Added comprehensive tests for access control, mint/burn, redeemAndBurn, and pause/unpause behavior. |
| test/enclaves/MarketV1.ts | Updated test assertions for job settlement and deposit/withdraw logic using block.timestamp values. |
| scripts/deploy/enclaves/UpgradeMarketV1.ts | Comments out deprecated reinitialization logic post-upgrade. |
| contracts/token/Credit.sol | Introduced PAUSER_ROLE and updated function documentation; adjusted redeemAndBurn sequence. |
| contracts/enclaves/MarketV1.sol | Redesigned job settlement and rate revision logic using block.timestamp and introduced maxRate. |
| addresses/421614.json | Updated MarketV1 implementation address. |
Comments suppressed due to low confidence (1)
contracts/token/Credit.sol:141
- Verify that burning tokens before transferring USDC is the intended sequence; ensure that if the USDC transfer fails after burning, the contract state remains consistent or appropriate error handling is in place.
function redeemAndBurn(address _to, uint256 _amount) external whenNotPaused onlyRole(REDEEMER_ROLE) {
| @@ -274,8 +277,8 @@ contract MarketV1 is | |||
| bytes32 jobId = bytes32(_jobIndex); | |||
|
|
|||
| // create job with initial balance 0 | |||
| jobs[jobId] = Job(_metadata, _owner, _provider, 0, 0, block.timestamp); | |||
| emit JobOpened(jobId, _metadata, _owner, _provider); | |||
| jobs[jobId] = Job(_metadata, _owner, _provider, 0, 0, block.timestamp, 0); | |||
There was a problem hiding this comment.
Consider initializing the maxRate field with the initial rate value (e.g. _rate) during job creation instead of 0. This would immediately reflect the actual job rate and simplify subsequent rate revision logic.
| jobs[jobId] = Job(_metadata, _owner, _provider, 0, 0, block.timestamp, 0); | |
| jobs[jobId] = Job(_metadata, _owner, _provider, 0, _rate, block.timestamp, 0); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
It is intentionally separated so that events are consistent across actions.
| const jobInfo = await marketv1.jobs(INITIAL_JOB_INDEX); | ||
| expect(jobInfo.metadata).to.equal("some metadata"); | ||
| expect(jobInfo.owner).to.equal(await user.getAddress()); | ||
| expect(jobInfo.provider).to.equal(await provider.getAddress()); | ||
| expect(jobInfo.rate).to.equal(JOB_RATE_1); | ||
| expect(jobInfo.balance).to.equal(initialBalance.sub(noticePeriodCost)); | ||
| expect(jobInfo.lastSettled).to.be.within(INITIAL_TIMESTAMP + FIVE_MINUTES, INITIAL_TIMESTAMP + FIVE_MINUTES + 1); | ||
| expect(jobInfo.lastSettled).to.equal(jobOpenTs); |
There was a problem hiding this comment.
Using strict equality for block timestamps might lead to brittle tests; consider using a tolerance window (e.g. to.be.closeTo) to account for minor timing variations.
| expect(jobInfo.lastSettled).to.equal(jobOpenTs); | |
| expect(jobInfo.lastSettled).to.be.closeTo(jobOpenTs, 1); |
Copilot uses AI. Check for mistakes.
This pull request introduces several updates to the
MarketV1smart contract, theCredittoken contract, and associated deployment scripts and tests. The changes focus on improving functionality, enhancing security, and aligning the implementation with updated requirements. Key updates include modifications to job settlement logic, the addition of new features in theCreditcontract, and updates to tests to reflect these changes.Updates to
MarketV1Contract:maxRatefield to theJobstruct to track the highest rate for a job. This field is updated when a new higher rate is set, and additional logic ensures sufficient balance to cover the notice period's extra cost. (contracts/enclaves/MarketV1.sol, [1] [2]JobOpenedandJobClosedto include atimestampfield, improving traceability. (contracts/enclaves/MarketV1.sol, [1] [2]_jobSettlefunction by removing thesettleTillparameter and usingblock.timestampdirectly. This change also ensures the job balance and rate are validated before settlement. (contracts/enclaves/MarketV1.sol, contracts/enclaves/MarketV1.solL287-R311)_emergencyWithdrawCreditto directly update the job balance and emit aJobWithdrawnevent, ensuring better transparency during emergency withdrawals. (contracts/enclaves/MarketV1.sol, contracts/enclaves/MarketV1.solL253-R263)Updates to
CreditContract:pauseandunpausefunctions with a newPAUSER_ROLE, allowing the contract to be paused for security or administrative reasons. (contracts/token/Credit.sol, contracts/token/Credit.solR133-R169)mint,burn, andredeemAndBurn, improving code clarity. (contracts/token/Credit.sol, [1] [2]USDCaddress public for better visibility. (contracts/token/Credit.sol, contracts/token/Credit.solL78-R83)Deployment and Testing Updates:
UpgradeMarketV1.tsdeployment script, as it is no longer required. (scripts/deploy/enclaves/UpgradeMarketV1.ts, scripts/deploy/enclaves/UpgradeMarketV1.tsL39-R42)MarketV1to validate the newmaxRatefield and ensurelastSettledtimestamps match the block timestamp when a job is opened. (test/enclaves/MarketV1.ts, [1] [2]These changes collectively enhance the robustness, clarity, and maintainability of the contracts and associated codebase.