AA-184: Remove second postOp retry

[forshtat]

No description provided.

[leekt]

if postOp retry is getting removed, shouldn't this be lowered to 2 instead of 3?
https://github.com/eth-infinitism/account-abstraction/blob/033b4be2a606defd5cd5226bdde3afcea21db5fa/contracts/core/EntryPoint.sol#L329C11-L329C11

[yoavw]

if postOp retry is getting removed, shouldn't this be lowered to 2 instead of 3? https://github.com/eth-infinitism/account-abstraction/blob/033b4be2a606defd5cd5226bdde3afcea21db5fa/contracts/core/EntryPoint.sol#L329C11-L329C11

I think so. @drortirosh @forshtat any reason not to?

[hazim-j]

What's the rationale behind removing postOp retry? Wouldn't this prevent paymasters from protecting their reputation incase something like a transfer doesn't work out?

[drortirosh]

Wouldn't this prevent paymasters from protecting their reputation incase something like a transfer doesn't work out?
removing the 2nd postOp means a paymaster can't revert entire bundle: it can revert the user's action (though he still pays for the execution's revert including the paymaster's revert)
This way, it actually protects its reputation...

The invariant we add is that passing all validation means transaction will always get paid for.

[drortirosh]

The change in the paymaster's code is in cases where it wants to perform different action upon reverting the user's execution.
Say a paymaster want to count the times it reverts user's code. In current model, it would revet in 1st postOp, and increment the counter in the 2nd postOp.

In the suggested new model, it will increment the counter in the validatePaymasterUserOp, and decrement it in the postOp.
While the flow looks a little strage, the outcome is the same in reverting and non-reverting flows.

The only thing that we know of that can be done in the two-postOp mode and can't be with the new model is emitting an event in the 2nd postOp. We argue that this is not that useful, and any app that want a log to track reverts can use UserOperationEvent instead.

[yoavw]

What's the rationale behind removing postOp retry? Wouldn't this prevent paymasters from protecting their reputation incase something like a transfer doesn't work out?

This mechanism was needed when validatePaymasterUserOp was a view function, and the paymaster couldn't do its own bookkeeping during validation. E.g. a TokenPaymaster couldn't use the precharge+refund model and had to charge in postOp.

With "view" being removed (long ago), paymasters can defend themselves during validation. A TokenPaymaster can precharge the max cost and then refund the unused part in postOp. A paymaster that wants to verify that the transaction performed some action and ban the user otherwise, could raise the "ban" flag for the user during validation and then reset it in postOp (or revert there, which leaves the user in a banned state. Any logic could be handled similarly.

As for protecting the paymaster's reputation, postOp becomes irrelevant for reputation purposes since it can never revert the bundle anymore. The bundler doesn't care if postOp reverts because it only reverts the UserOp, and the paymaster still pays for it. Therefore such reverts don't affect the paymaster's reputation anymore.

[drortirosh]

missing fix for the code that calls postOp with postOpReverted (in executeUserOp

[forshtat]

missing fix for the code that calls postOp with postOpReverted (in executeUserOp

@drortirosh please elaborate what specifically do you think requires fixing?

Still checking postOpReverted mode

[shahafn]

Why do we still need mode here?

[shahafn]

Maybe change _handlePostOp() to _postExecution()? It's no longer handling post op only

[drortirosh]

in case the PostOp reverts, the "outer" postExecution will report the error (success=false), but there is no indication that it was a paymaster (postOp) that reverted, instead of user-code that reverted.
I suggest emit PostOpRevertReason to report it.
(technically, it is not here, but at the "catch" of the innerHandleOp)