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

Lib CallbackUtils & Remove PoolNFT callback try/catch #1979

Merged
merged 43 commits into from
Jul 4, 2024
Merged

Conversation

hellwolf
Copy link
Contributor

@hellwolf hellwolf commented Jul 2, 2024

  • CallbackUtils replaces SafeGasLibrary.
  • Remove try/catch in PoolNFT callbacks.

@hellwolf hellwolf requested review from d10r and a team as code owners July 2, 2024 13:03
@hellwolf hellwolf linked an issue Jul 2, 2024 that may be closed by this pull request
1 task
Copy link

github-actions bot commented Jul 2, 2024

Changelog Reminder

Reminder to update the CHANGELOG.md for any of the modified packages in this PR.

  • CHANGELOG.md modified
  • Double check before merge

@hellwolf hellwolf marked this pull request as draft July 2, 2024 13:04
@hellwolf hellwolf linked an issue Jul 3, 2024 that may be closed by this pull request
1 task
@hellwolf hellwolf marked this pull request as ready for review July 3, 2024 20:54
@hellwolf hellwolf changed the title Lib CallbackUtils Lib CallbackUtils & Remvoe PoolNFT callback try/catch Jul 3, 2024
Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.62%. Comparing base (60dc875) to head (75cbb57).
Report is 2 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1979      +/-   ##
==========================================
+ Coverage   88.53%   88.62%   +0.09%     
==========================================
  Files         119      119              
  Lines        7389     7387       -2     
  Branches      977      980       +3     
==========================================
+ Hits         6542     6547       +5     
+ Misses        845      838       -7     
  Partials        2        2              
Flag Coverage Δ
ethereum-contracts 94.22% <100.00%> (+0.18%) ⬆️
sdk-core 88.62% <100.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hellwolf hellwolf enabled auto-merge July 4, 2024 00:50
Copy link
Collaborator

@d10r d10r left a comment

Choose a reason for hiding this comment

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

Looks good.
I was most concerned about poolMemberNFT.onDelete failing if minting hadn't succeeded. But that's safe because if not minted, poolMemberNFT.poolMemberDataByTokenId(tokenId).member will be the zero address.
This way we get rid of the issue of try/catch misleading the gas estimation, leading to failed transactions.

This PR also fixes the failing test in #1982 (I tried locally).

@hellwolf hellwolf added this pull request to the merge queue Jul 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 4, 2024
@hellwolf hellwolf added this pull request to the merge queue Jul 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 4, 2024
@hellwolf hellwolf requested a review from kasparkallas as a code owner July 4, 2024 10:23
@hellwolf hellwolf enabled auto-merge July 4, 2024 10:23
@d10r d10r changed the title Lib CallbackUtils & Remvoe PoolNFT callback try/catch Lib CallbackUtils & Remove PoolNFT callback try/catch Jul 4, 2024
@hellwolf hellwolf added this pull request to the merge queue Jul 4, 2024
Merged via the queue into dev with commit 73705c7 Jul 4, 2024
20 checks passed
@hellwolf hellwolf deleted the lib-callback-utils branch July 4, 2024 12:22
Copy link

github-actions bot commented Jul 4, 2024

XKCD Comic Relif

Link: https://xkcd.com/1979
https://xkcd.com/1979

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants