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

bugfix: Fix native currency tip amount scaling logic for partial fills when using fulfillOrders #565

Merged

Conversation

naveen-imtb
Copy link
Contributor

Motivation

Fixes 564

Solution

The tip amount scaling logic has been moved into the fulfillAvailableOrders function where all the order's offer and consideration items are scaled proportional to the ratio of the order being filled.

This ensures that the fulfillAvailableOrders function has access to the original (unscaled) tip amounts to be passed to the Seaport contract as consideration items.

@naveen-imtb naveen-imtb changed the title bugfix: move tips scaling logic to where offer and consideration items are scaled. bugfix: Fix native currency tip amount scaling logic for partial fills when using fulfillOrders Jun 5, 2024
@ryanio
Copy link
Collaborator

ryanio commented Jun 10, 2024

thank you for the PR! could you add a test that helps validate that the math/scaling log is correct now? bonus points if it fails prior to the source change and passes after it :)

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 98.61111% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.42%. Comparing base (f618d19) to head (3962a76).
Report is 74 commits behind head on main.

Files Patch % Lines
src/utils/fulfill.ts 96.15% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
+ Coverage   98.27%   98.42%   +0.14%     
==========================================
  Files          35       35              
  Lines       14526    15013     +487     
  Branches      660      675      +15     
==========================================
+ Hits        14276    14776     +500     
+ Misses        245      232      -13     
  Partials        5        5              

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

@naveen-imtb
Copy link
Contributor Author

thank you for the PR! could you add a test that helps validate that the math/scaling log is correct now? bonus points if it fails prior to the source change and passes after it :)

Hi @ryanio, thank you for reviewing the PR.

Regarding your comment - my understanding is that the actual scaling happens in the smart contract and not in the SDK. In the case of partial fills, the on-chain smart contract uses the offer and consideration parameters that are submitted to the fulfillAvailableAdvancedOrders() in conjunction with the numerator and denominator values to determine the proposed fill ratio and scale down the consideration items and tips accordingly.

Therefore if incorrect order parameters, tips in the case of this specific bug, are passed to fulfillOrders() then the on-chain smart contract function fulfillAvailableAdvancedOrders() is called with those incorrect order parameters resulting in the scaled tip amounts to be inline with the incorrect inputs.

The expected assertion here would be for the unit test to verify that the correct order parameters (i.e the original form of the order) are passed to the fulfillOrders() SDK function to ensure that the correct values are used when invoking the on-chain smart contract function.

I've now extended the test assertion to verify the function arguments.

As per your request, created another PR to verify the presence of the bug, failing test assertion in the absence of the correct tip amounts scaling fix.

@ryanio ryanio merged commit b349422 into ProjectOpenSea:main Jun 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect tip amount scaling (only native currency) in partial fill scenario when using fulfillOrders
4 participants