-
Notifications
You must be signed in to change notification settings - Fork 48
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
Multiple refactors (SegmentWithDelta
, broker.account
, range unpack, order of params)
#334
Conversation
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.
dea3688
to
98c0d5e
Compare
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.
The deltas
var can be deleted here:
https://github.com/sablierhq/v2-core/blob/98c0d5efd0c924b0bb3cc929d2de5affce6862d6/test/invariant/handlers/LockupProCreateHandler.t.sol#L74
You forgot to update the notice natspec for Range
structs, they are not used anymore in the Stream
https://github.com/sablierhq/v2-core/blob/98c0d5efd0c924b0bb3cc929d2de5affce6862d6/src/types/DataTypes.sol#L62
https://github.com/sablierhq/v2-core/blob/98c0d5efd0c924b0bb3cc929d2de5affce6862d6/src/types/DataTypes.sol#L96
It is not about this PR, but I noticed it now. In the BootstrapProtocol
the first stream isn't depleted because the end time can't be equal to the start time.
test/unit/lockup/linear/create-with-durations/createWithDurations.t.sol
Outdated
Show resolved
Hide resolved
Thanks for reviewing this, @andreivladbrg. I agree with all of your proposed changes.
Good catch, though this isn't strictly the case. It is possible that Forge will process the stream creation and the stream withdrawal in two different blocks, which would deplete the stream when Nonetheless, I agree that we should remove that call. The solution is to use |
98c0d5e
to
0e19ff1
Compare
ea267ee
to
dfe9484
Compare
chore: improve wording in code comments perf: mark helper functions as "pure" refactor: delete "SegmentArrayCountsNotEqual" error refactor: reorder params in "create" functions in pro contract refactor: use "SegmentWithDelta" struct in "createWithDeltas" function test: add "DEFAULT_SEGMENTS_WITH_DELTAS" constant test: add "getSegmentsWithMilestones" util in "Utils.sol" test: add overloads for "fuzzSegmentAmountsAndCalculateCreateAmounts" test: delete tests related to "SegmentArrayCountsNotEqual" error test: delete unused "sdUint128" and "sdUint40" functions test: move "getBlockTimestamp" to "Utils.sol" test: rename "vars.amounts" to "vars.createAmounts"
test: update the tests to match the latest contract API
chore: remove "withdrawMax" call in the bootstrap test: remove unused "deltas" variables test: use "DEFAULT_SEGMENTS" values in "DEFAULT_SEGMENTS_WITH_DELTAS" test: use the assert stream function in the unit tests
dfe9484
to
713bd1d
Compare
@andreivladbrg I will merge this PR now. The end-to-end tests have failed, but this is because the fuzzing campaign used an address that is blacklisted by USDC (#317). |
Description
Closes #328, #329, #330, and #333.
I have implemented #333 rather grudgingly - as I said in #310, I don't like it that there are now multiple ways to access the times: one via the
Range
struct (e.g. ingetRange
) and another via theLockupLinear.Stream
struct (e.g. ingetStream
). But I agree that 20k gas is not insignificant.Regarding #328 - it would be good if @razgraf could also take a look at the new API implemented by this PR, before we merge it.
Changelog