superfluid-finance / protocol-monorepo

Superfluid Protocol Monorepo: the specification, implementations, peripherals and development kits.
https://www.superfluid.finance
Other
875 stars 239 forks source link

[ETHEREUM-CONTRACTS] flow nft hooks can fail with outofgas #1879

Closed d10r closed 8 months ago

d10r commented 8 months ago

What & Why

The FlowNFT hooks were implemented wrapped in try/catch out of caution, because we don't want the whole tx to fail for any reason related to NFT minting/burning.
The intention was to have the same behaviour as SuperApp hooks: if the hook invocation reverts with outofgas, the whole tx should revert. If it reverts for other reasons, the tx should not revert.

Yet we see non-reverting transactions with the NFT hook reverting due to outofgas, for example this.

This is likely to happen due to the way geth gas estimation works: it does a binary search for the lower limit of gas required (see source).

AC

d10r commented 8 months ago

fork testing: https://github.com/d10r/sf-flownft-hook-change-test

Missing: