[ETHEREUM-CONTRACTS] Fix nfthooks outofgas #1880
Conversation
Changelog ReminderReminder to update the CHANGELOG.md for any of the modified packages in this PR.
|
| _warpAndAssertAll(superToken); | ||
|
|
||
| _helperCreateFlow(superToken, bob, alice, flowRate); | ||
|
|
||
| _warpAndAssertAll(superToken); | ||
| } | ||
|
|
||
| // there should be no gas limit which causes the NFT hook to fail with the tx succeeding | ||
| function testNFTHookOutOfGasRevertsWholeTx(uint256 gasLimit) public { |
There was a problem hiding this comment.
did you observe any failure without the fix with such fuzzing?
There was a problem hiding this comment.
yes, reliably failed.
Example:
[FAIL. Reason: CFA_NFT_INVALID_TOKEN_ID(); counterexample: calldata=0xa34c991a00000000000000000000000000000000000000000000000000000000000060c4 args=[24772 [2.477e4]]] testNFTHookOutOfGasRevertsWholeTx(uint256) (runs: 27, μ: 415430, ~: 419640)
Logs:
Bound Result 424774
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 55.35ms
|
Question without reviewing it holistically yet: if it deletes a NFT that didn't exist (minting failed), would this revert? @d10r @0xdavinchee |
|
https://github.com/d10r/sf-flownft-hook-change-test |
…case a previously existing one wasn't burned on flow delete)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1880 +/- ##
==========================================
+ Coverage 89.35% 89.42% +0.07%
==========================================
Files 110 110
Lines 5920 5903 -17
Branches 216 216
==========================================
- Hits 5290 5279 -11
+ Misses 578 572 -6
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
XKCD Comic RelifLink: https://xkcd.com/1880 |
The fuzz test nicely reproduces the issue.
I couldn't fix it with the try/catch in place.
My proposal is to remove the try/catch. The execution is conditional on the FlowNFT addresses being set, so won't be attempted if none is set. As far as I remember, the decision to try/catch those hooks was not taken out of conviction, but more in a "why not" way, assuming it won't cause any issues.