Conversation
Summary by OctaneNew ContractsNo new contracts were added. Updated Contracts
🔗 Commit Hash: 26d9c35 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ 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 |
Use parameter registry key
|
neekolas
force-pushed
the
01-27-get_start_time_from_parameter_registry
branch
from
26d9c35 to
4aef029
Compare
Changes to gas cost
🧾 Summary (20% most significant diffs)
Full diff report 👇
|
LCOV of commit
|
OverviewOctane AI analysis has finished. No vulnerabilities were found. Cheers! 🎉🎉🎉 🔗 Commit Hash: 26d9c35 |
neekolas
force-pushed
the
01-27-get_start_time_from_parameter_registry
branch
2 times, most recently
from
f9be91b to
94d1a95
Compare
neekolas
force-pushed
the
01-27-get_start_time_from_parameter_registry
branch
from
94d1a95 to
8af46e4
Compare
Slither 0.11.5 introduced a new reentrancy-balance detector that flags a pre-existing pattern in DistributionManager._makeWithdrawableAmount(). Tracked in #258. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
KevinSmall
left a comment
KevinSmall
left a comment
There was a problem hiding this comment.
lgtm, some minor comments
| uint64 startTime_ = RegistryParameters.getUint64Parameter(parameterRegistry, ratesInEffectAfterParameterKey()); | ||
|
|
||
| uint64 startTime_ = uint64(block.timestamp); | ||
| _revertIfNoRateChange(messageFee_, storageFee_, congestionFee_, targetRatePerMinute_); |
There was a problem hiding this comment.
Do we want a change in just the startTime_ to count as a valid change? At present if we changed just the startTime_ it'd count as "no rate change" here, and the transaction would revert. Which might be fine.
There was a problem hiding this comment.
Great question. I initially had it in the calculation and Octane made some very good points.
The concern is that if the startTime moved ahead of other parameters and updateRates got called you could end up with duplicate rates (old parameters new start time). And then when it did get called again you would end up with two rates that have the same start time.
| ); | ||
|
|
||
| _revertIfNoRateChange(messageFee_, storageFee_, congestionFee_, targetRatePerMinute_); | ||
| uint64 startTime_ = RegistryParameters.getUint64Parameter(parameterRegistry, ratesInEffectAfterParameterKey()); |
There was a problem hiding this comment.
It might be overkill to add this in, but we could add validation here to see if startTime_ were set to something unexpected, like in the past?
If we can, I'd favor trusting our admin process to get it right, and keep contract simple.
There was a problem hiding this comment.
The one validation that seems the most important is that the start time of the new rates is after the start time of the previous rates. The nodes will reject out of order rate changes, but it would be nice to cut them off at the source.
neekolas
force-pushed
the
01-27-get_start_time_from_parameter_registry
branch
from
da81345 to
e23e41d
Compare
| @@ -1,4 +1,4 @@ | |||
| { | |||
| "detectors_to_exclude": "", | |||
| "detectors_to_exclude": "reentrancy-balance", | |||
There was a problem hiding this comment.
I forgot to mention, we could drop this change if we wanted.
Another PR pinned slither in CI and so the CI error has gone away.
Ensure new rate start times are strictly increasing to prevent out-of-order rate entries. Validation is skipped on first update when the rates array is empty. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
|
||
| /// @inheritdoc IIdentified | ||
| function version() external pure returns (string memory version_) { | ||
| return "1.0.0"; |
There was a problem hiding this comment.
| return "1.0.1"; |
Oops, I forgot about the version bump
2 participants
TL;DR
Added a new parameter to control when rate changes take effect in the RateRegistry contract.
What changed?
xmtp.rateRegistry.ratesInEffectAfterto the RateRegistry_setRateRegistryStartingRates()function to include the new parameterHow to test?
test_updateRates_ratesInEffectAfterOutOfTypeBoundstest case to ensure proper validationDeployLocal.s.solscript and verify that rates are set with the correct effective timestampWhy make this change?
This change allows for more flexible control over when rate changes take effect, rather than always using the current block timestamp. This is particularly useful for coordinating rate changes across different systems or for scheduling future rate updates, providing better predictability and control over the rate change process.