Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks
d10r opened this issue Mar 6, 2024 · 1 comment · Fixed by #1880
Closed
2 tasks

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

d10r opened this issue Mar 6, 2024 · 1 comment · Fixed by #1880
Assignees
Labels
Project: PROTOCOL-EVMv1 Superfluid protocol EVM v1 implementation in Solidity Type: Bug Something isn't working

Comments

@d10r
Copy link
Collaborator

d10r commented Mar 6, 2024

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

  • test case reproducing the issue, ideally by fuzzing
  • fix
@d10r d10r added Type: Bug Something isn't working Project: PROTOCOL-EVMv1 Superfluid protocol EVM v1 implementation in Solidity Team: Protocol Protocol Core, Sentinel, Peripherals, Protocol Infrastructure Tools & DevOps labels Mar 6, 2024
@d10r d10r self-assigned this Mar 6, 2024
@d10r d10r linked a pull request Mar 6, 2024 that will close this issue
@hellwolf hellwolf removed the Team: Protocol Protocol Core, Sentinel, Peripherals, Protocol Infrastructure Tools & DevOps label Mar 11, 2024
@d10r
Copy link
Collaborator Author

d10r commented Mar 15, 2024

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

Missing:

  • also apply for GDA
  • consider scenario of flow now existing, but NFT existing (previous delete hook failing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Project: PROTOCOL-EVMv1 Superfluid protocol EVM v1 implementation in Solidity Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants