fix: include tips in the fulfiller's balance and approval checks - #962
Open
Dusk1e wants to merge 1 commit into
Open
fix: include tips in the fulfiller's balance and approval checks#962Dusk1e wants to merge 1 commit into
Dusk1e wants to merge 1 commit into
Conversation
fulfillOrder and fulfillOrders collect the fulfiller's balances and approvals from the order's offer and consideration items only, but the checks that consume them sum tips in alongside the consideration. When a tip names a token or identifier the order does not already carry, there is no entry to look up and findBalanceAndApproval throws Checking for balance and approvals for token 0x... id 0 failed before a transaction is ever built. That rules out ordinary tips: a native ETH tip on an ERC20-denominated listing, a tip in a second ERC20, or an NFT tip. Include the mapped tip items in the fulfiller's lookup set so the tips are actually checked rather than crashing the lookup. Tips are appended after the order's items, so the criteria mapping the existing items rely on is unchanged. This also means a tip in a token the fulfiller has not approved now surfaces its own approval action, which the fulfillment would otherwise revert without.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
fulfillOrderandfulfillOrdersbuild the fulfiller's balance and approval set from the order's own items only:The checks that read that set sum tips in alongside the consideration. So a tip in a token the order does not already carry has no entry to look up, and
findBalanceAndApprovalthrows before any transaction is built:That rules out a native tip on an ERC20 listing, a tip in a second ERC20, and NFT tips, all of which Seaport itself accepts. Tipping in the same token as the order works fine, which is why the existing tip coverage in
partial-fulfill.spec.tsnever hit this.Fix
Include the mapped tip items in the fulfiller's lookup set in both entry points. They go on the end, after the order's items, so the offer and consideration criteria mapping in
getItemToCriteriaMapis untouched.It also makes the check do its job rather than just stop crashing: a tip in a token the fulfiller has not approved now produces its own approval action, which the fulfillment would revert without.
In
fulfillOrdersthe per-order tip mapping is hoisted so it is built once and reused for both the lookup set andordersMetadata, instead of mapped twice.Test
test/tips.spec.tscovers both entry points. Both fail onmainwith the error above.fulfillOrderwith a second-ERC20 tip: asserts both approval actions appear, then fulfills and checks the NFT and the tip both land.fulfillOrderswith a native tip on an ERC20 listing: asserts the tip rides asmsg.value, then fulfills and checks the recipient's balance moved.Suite green at 174.