Make max end hints a custom type #346
Conversation
CodeSandwich
force-pushed
the
igor/splits_api
branch
from
e87a334 to
24b2f22
Compare
CodeSandwich
force-pushed
the
igor/hints
branch
from
c877a22 to
5bd52c3
Compare
CodeSandwich
force-pushed
the
igor/splits_api
branch
from
24b2f22 to
30be8e4
Compare
src/Streams.sol
Outdated
| /// @param maxEndHint1 An optional parameter allowing gas optimization, pass `0` to ignore it. | ||
| /// The first hint for finding the maximum end time when all streams stop due to funds | ||
| /// @param maxEndHints An optional parameter allowing gas optimization. | ||
| /// Pass a list of 8 zero value hints to ignore it, it's represented as an integer `0`. |
There was a problem hiding this comment.
| /// Pass a list of 8 zero value hints to ignore it, it's represented as an integer `0`. | |
| /// Pass `0` to ignore it. |
That's too complicated in my opinion list of 8 zero value hints for the reader who only wants to skip it.
src/Streams.sol
Outdated
| /// Hints lower than the current timestamp including the zero value hints are ignored. | ||
| /// If you provide fewer than 8 non-zero value hints make them the rightmost values to save gas. | ||
| /// It's the best approach to make the most risky and precise hints the rightmost ones. |
There was a problem hiding this comment.
I think it would good to mention specifically that there are no assumptions.
- no specific ordering of the hints is strictly required
- only the already mentioned gas improvement with risky hints on the right
- all hints could be used to improve either higher bound or the lower bound
- it is not required that hints come in pairs of two with a start and end
- provided list only improves gas usage and has no impact on the outcome of the transaction
I think a potentially confusing part is: why is it a list with multiple hints.
Maybe a few strategies to illustrate the concept. Alternatively a link to some documentation
For example: Unkown block.timstamp of the new transactions together with streams which should stop when running out of funds:
- risky hint pair a
1 minstart/end range - 5 minute hint pair start/end range
- 2 hours pair for the worst case
| if (hint2 > enoughEnd && hint2 < notEnoughEnd) { | ||
| if (_isBalanceEnough(balance, configs, configsLen, hint2)) { | ||
| enoughEnd = hint2; | ||
| while (maxEndHints.hasHints()) { |
There was a problem hiding this comment.
Just to double check.
uint256 enoughEnd = _currTimestamp();Current timestamp will be always enoughEnd. This is an invariant of the system and can never be false.
| (maxEndHints, hint) = maxEndHints.pop(); | ||
| if (hint <= enoughEnd || hint >= notEnoughEnd) continue; | ||
| if (_isBalanceEnough(balance, configs, configsLen, hint)) { | ||
| enoughEnd = hint; |
There was a problem hiding this comment.
Logic makes sense to me 👍
| /// @param hints The list of hints. | ||
| /// @return newHints The modified list of hints. | ||
| /// @return hint The removed hint. | ||
| function pop(MaxEndHints hints) internal pure returns (MaxEndHints newHints, uint32 hint) { |
There was a problem hiding this comment.
It makes sense to implement it with a pop instead of a read operation for a specific list index. (more costly)
This makes the calldata simpler by replacing zero-padded 4-byte words with a full word. The API also becomes cleaner by reducing the number of parameters.