Merge account abstraction work into master #5274
Conversation
Co-authored-by: Hadrien Croubois <[email protected]>
Co-authored-by: cairo <[email protected]> Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: cairo <[email protected]> Co-authored-by: Ernesto García <[email protected]> Co-authored-by: Arr00 <[email protected]>
🦋 Changeset detectedLatest commit: 39dc802 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| } | ||
|
|
||
| /// @dev Returns the aggregator of the `validationData` and whether it is out of time range. | ||
| function getValidationData(uint256 validationData) internal view returns (address aggregator, bool outOfTimeRange) { |
There was a problem hiding this comment.
Would it be better to return an "in range" than an "out of range" boolean ?
There was a problem hiding this comment.
I'd agree with this change too. It feels weird the boolean is indicating an undesired state
| * The `aggregator` is set to {SIG_VALIDATION_SUCCESS} if both are successful, while | ||
| * the `validAfter` is the maximum and the `validUntil` is the minimum of both. | ||
| */ | ||
| function combineValidationData(uint256 validationData1, uint256 validationData2) internal pure returns (uint256) { |
There was a problem hiding this comment.
Do we need this function. AFAIK the original usecase is aggreagation in multisig accounts, but it may not be relevant anymore.
Feels quite opinionated. In particular, maybe if aggregator1 == aggregator2 that should be ok.
There was a problem hiding this comment.
Yes, I'd agree with removing it. Can we still do it now that is merged?
No description provided.