Skip to content

ValidationManager account for signature expiration#775

Merged
InoMurko merged 11 commits intodevelopfrom
wsdt/valmgr-expireuop
May 5, 2023
Merged

ValidationManager account for signature expiration#775
InoMurko merged 11 commits intodevelopfrom
wsdt/valmgr-expireuop

Conversation

@wsdt
Copy link
Copy Markdown
Contributor

@wsdt wsdt commented May 3, 2023

📋 Add associated issues, tickets, docs URL here.

Overview

ValidationManager needs to account for expiring UserOp
Resolves #753

Changes

  • Reversing >< as probably semantic mistake.
  • Added validAfter requireCondition as it was missing altogether
  • Added two integration tests to cover validAfter and validUntil
  • Adding two unit tests to cover validAfter and validUntil (in progress)

Testing

AA + bundler tests run through.

@wsdt wsdt requested review from InoMurko and souradeep-das May 3, 2023 15:32
@InoMurko
Copy link
Copy Markdown
Contributor

InoMurko commented May 3, 2023

Okay - so this needs a bit more then uncommenting code.

First, you can see there's no test coverage for this code path, not in unit tests and not in integration tests. Once that's added we'll have more understanding of the PR.

@InoMurko
Copy link
Copy Markdown
Contributor

InoMurko commented May 3, 2023

The reason why this is more complex is the divergence of AA contracts and bundler. @souradeep-das has more to share here.

Comment thread packages/boba/bundler/src/modules/ValidationManager.ts Outdated
Copy link
Copy Markdown
Contributor

@souradeep-das souradeep-das left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, I think the switch of the operator should make it work! (more precisely maybe the expression added as comment?)

this condition comes up in verifyingPaymaster/BobaVerifyingPaymaster (integ test file - boba_aa_sponsoring_fee.test.ts) would be great, if we could add an extra test case to the file covering the validUntil/validAfter case

in case we wanted to add a unit-test, we could add one to ValidateManager.test.ts (modifying paymsterData to include validationData)

Comment thread packages/boba/bundler/src/modules/ValidationManager.ts Outdated
Comment thread packages/boba/bundler/src/modules/ValidationManager.ts Outdated
Comment thread packages/boba/bundler/src/modules/ValidationManager.ts Outdated
@wsdt wsdt force-pushed the wsdt/valmgr-expireuop branch from ff3bf7a to 58a1859 Compare May 4, 2023 17:10
@wsdt wsdt force-pushed the wsdt/valmgr-expireuop branch from 8c87937 to 8a8455a Compare May 4, 2023 18:08
@wsdt wsdt requested a review from souradeep-das May 4, 2023 18:53
Copy link
Copy Markdown
Contributor

@souradeep-das souradeep-das left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! 🚀

// not adding "buffer" here
res.returnInfo.validAfter < Date.now() / 1000,
'not valid yet',
ValidationErrors.NotValidYet,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the doc with the new error code (https://github.com/bobanetwork/boba/pull/778)

this is better, but just wondering if we should squeeze this into the -32503 error code? (referencing the error codes in the spec https://github.com/ethereum/EIPs/blob/master/EIPS/eip-4337.md)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point!

Well both are fine for me, advantage would be to stick more to the original spec but disadvantage would be that you don't know if the signature expired or will be valid in future when merging them together (and the error message might be misleading too except we change that altogether).

ExpiresShortly = -32503

@wsdt wsdt requested a review from InoMurko May 5, 2023 11:17
Copy link
Copy Markdown
Contributor

@InoMurko InoMurko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERY GOOD

@InoMurko InoMurko merged commit ef02bee into develop May 5, 2023
@InoMurko InoMurko deleted the wsdt/valmgr-expireuop branch May 5, 2023 11:48
InoMurko added a commit that referenced this pull request May 8, 2023
* resolve #753

* Update packages/boba/bundler/src/modules/ValidationManager.ts

Co-authored-by: Ino Murko <ino.murko.github@protonmail.com>

* fix bool

* validAfter/Until integrationt tests, validAfter

* cleanup

* regex

* integration_tests

* integration & unit tests

---------

Co-authored-by: Ino Murko <ino.murko.github@protonmail.com>
(cherry picked from commit ef02bee)
InoMurko added a commit that referenced this pull request May 8, 2023
* Inomurko/bump bundler  (#698)

* bump bundler, limit dependency bumps

* uncomment bundler related stuff

* build bundler in docker

* build fixes

* fix tests

* fix bundler building

* uncomment _disableInitializers

* fix running DTL

* v1.0.0

* fixing starting bundler and tests, default config

* local unsafe, linting fix in intg tests

* custom errors fixes

* new api for simple account contract

* use simple account factory proxy

* use simple account factory proxy in SimpleAccountAPI

* use simple account factory proxy in SimpleAccountAPI

* use wrappers to get around custom errors

* update entrypoint wrapper (#745)

* sponsoring fee fixed

* stricter validation for staking

* remove debug namespace, fix return for unaavailable rpc methods

* addressing Souradeeps comments

---------

Co-authored-by: Souradeep Das <dsouradeep2@gmail.com>
(cherry picked from commit 6368c72)

* fix: qsp30 (#773)

fix: BOB1-30
(cherry picked from commit 7feda88)

* run op fix (#771)

(cherry picked from commit 176cd3c)

* close-server (#768)

(cherry picked from commit 72021af)

* [AA]: fix inconsistent userOpHash (#757)

* add token callback handler on SimpleAccount

* fix: userOpHash packing

* prevent recursive calls into handleOps

* move nonce validation from individual Account to EntryPoint

* add bundler changes for nonce change to EP

(cherry picked from commit cc4e205)

* ValidationManager account for signature expiration (#775)

* resolve #753

* Update packages/boba/bundler/src/modules/ValidationManager.ts

Co-authored-by: Ino Murko <ino.murko.github@protonmail.com>

* fix bool

* validAfter/Until integrationt tests, validAfter

* cleanup

* regex

* integration_tests

* integration & unit tests

---------

Co-authored-by: Ino Murko <ino.murko.github@protonmail.com>
(cherry picked from commit ef02bee)

* npm release workflow for bundler-sdk (#749)

(cherry picked from commit 718141f)

* Fix/banxa and bridges (#772)

* adding boba network

* fixing boba bridge url

* fixing bridge integration

* replace code by selectors

* remove hardcoded symbol

* enable banxa only for mainnet

* Available bridge inable only for mainnet

* adding support for testnet

* update conditional for other bridges

* implemented the available bridges with typescript

* unit test cases for available bridges

* typo in Available bridges

---------

Co-authored-by: alvaro-ricotta <alvaro.e.ricotta@gmail.com>
Co-authored-by: alvaro-ricotta <81116391+alvaro-ricotta@users.noreply.github.com>
Co-authored-by: Ino Murko <ino.murko.github@protonmail.com>
(cherry picked from commit dcf9b7e)

* add validation of entryPoint and wrapper (#779)

* add validaiton of entryPoint and wrapper

(cherry picked from commit 41f3150)

---------

Co-authored-by: Souradeep Das <dsouradeep2@gmail.com>
Co-authored-by: Riedl Kevin, Bsc <kevin.riedl@wavect.io>
Co-authored-by: Sahil K <86316370+sk-enya@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[AA] ValidationManager needs to account for expiring UserOp

3 participants