ProjectOpenSea / seaport-js

A TypeScript library to interface with the Seaport marketplace.
MIT License
250 stars 182 forks source link

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

Closed naveen-imtb closed 1 month ago

naveen-imtb commented 1 month ago

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.

ryanio commented 1 month ago

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 commented 1 month ago

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 :warning:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

naveen-imtb commented 1 month ago

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.