feat: list collections through the newest off-chain marketplace - #3473
feat: list collections through the newest off-chain marketplace#3473LautaroPetaccio wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
a1571fe to
ca20d0d
Compare
Coverage Report for CI Build 33674668662Coverage increased (+0.02%) to 53.755%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
I reviewed this together with marketplace-squid-core#112, trades-squid-core#43, marketplace-server#395, decentraland-dapps#819, and builder#3472 for the V3 off-chain marketplace rollout.
Requesting changes for one correctness issue in Builder's off-chain sale authorization flow:
- P1 —
enableSaleOffchain()only targets the latest marketplace address for both enabling and disabling.isEnableForSaleOffchain()correctly treats any deployed off-chain marketplace version as an active listing minter, but disabling returns a revoke access for only the latest version. After the V3 rollout, a collection that still has V2 minter rights will continue to be sellable via existing V2 listings even after the user toggles off-chain sale off. Please make the disable path revoke every currently-authorized off-chain marketplace address, while keeping the enable path on the latest/signable marketplace.
Security pass: no secrets or direct injection issues found in this PR.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
LautaroPetaccio
left a comment
LautaroPetaccio
left a comment
There was a problem hiding this comment.
Good catch — that was a real bug and it was mine. Fixed in f413bbe.
Widening isEnableForSaleOffchain() to accept any deployed version without widening the revoke left the toggle half-broken: a collection holding V2 minter rights kept them after disabling, so existing V2 listings stayed sellable, and isEnableForSaleOffchain() still reported the collection as on sale — so the toggle looked like it had done nothing.
Worth noting the original V1 → V2 rollout got this right: 37531cbb revoked whichever version the collection actually held, and d2da9a66 later trimmed it to the single-address one-liner I inherited.
Now:
// Enabling grants the newest version only: the minter has to be the same contract as the
// signature domain or the mint reverts.
if (isOnSale) {
return [{ address: getLatestOffchainSale(chainId).address, hasAccess: true, collection }]
}
const authorized = getOffchainSaleAddresses(chainId).filter(address => includes(collection.minters, address))
const toRevoke = authorized.length ? authorized : [getLatestOffchainSale(chainId).address]
return toRevoke.map(address => ({ address, hasAccess: false, collection }))The fallback is deliberate: handleSetCollectionMintersRequest turns this list into the addresses/values arrays for setMinters, so returning [] when a collection holds no off-chain minters would send the contract two empty arrays. Falling back to the newest keeps the previous single-entry shape for that edge case.
Two tests added — that both held versions are revoked, and that a version the collection does not hold is not. 38 passing, tsc and eslint clean.
f413bbe to
c0b34a1
Compare
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
I re-reviewed the stacked V3 listing changes after the revoke fix and the decentraland-dapps@29.7.1 bump. The previous revoke-every-held-version issue is fixed, but there is still one blocking signing-path mismatch.
Findings
- P1 — Builder still signs primary item orders against
OffChainMarketplaceV2locally.enableSaleOffchain(..., true)now authorizesgetLatestOffchainSale(chainId), and the added regression checks that this matchesdecentraland-dapps'getLatestOffChainMarketplaceContract(). However Builder's actual item-order creation still uses the vendoredsrc/lib/trades.ts#getTradeSignature(), which hardcodesgetContract(ContractName.OffChainMarketplaceV2, trade.chainId)for the EIP-712 domain. On Amoy, the modal/authorization path can grant V3 minter rights while the created trade signature is still for V2, so the authorized minter and settlement/signature domain can diverge and primary listings can revert at mint. Please update the local signer to use the same latest-marketplace resolver (or remove the vendored signer once dapps supports the required USD-pegged path), and add/adjust a regression test that verifies the contract used by Builder's actualgetTradeSignature()matches the address authorized bygetLatestOffchainSale().
Security pass: no secrets, auth/authz, input-handling, or direct injection issues found in the changed code.
Consumer impact: no external API surface is changed; this is internal Builder marketplace-selection/signing behavior.
CI status: Vercel and audit are passing; one test job was still pending when checked, while the other test job passed.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
c0b34a1 to
e709b57
Compare
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest commit.
The previous P1 signing-path mismatch is fixed: Builder now creates item-order signatures through decentraland-dapps@29.7.1 instead of the vendored src/lib/trades.ts, and the new regression asserts the actual EIP-712 verifying contract matches the marketplace Builder authorizes. I also checked decentraland-dapps@29.7.1: getOffChainMarketplaceContract() now resolves through the same latest-marketplace helper, so the signature indexes and signature domain stay aligned.
No blocking findings.
Non-blocking follow-up suggestion:
- P2:
src/modules/collection/utils.tsstill duplicates the latest marketplace resolution order. The regression covers the current V3/V2 rollout, but using the dapps resolver directly for the “latest/signing” path in a future cleanup would reduce drift risk when another marketplace version is introduced.
Security pass: no secrets, auth/authz, input-handling, or direct injection issues found in the changed code.
Consumer impact: no external API surface is changed; this remains internal Builder marketplace-selection/signing behavior.
CI status: passing.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
|
✅ Approved by Claude, approved by Codex — resolves the off-chain marketplace version dynamically (V3 on testnet, V2 on mainnet) for granting/checking/revoking minter rights, and deletes the vendored trade-signing module in favor of dapps 29.7.1, which carries the USD-pegged fix and the same resolver. Checked: full diff and changed files; authorization/signing call sites; dependency bump (dapps 29.7.1 tarball verified to export Non-blocking, agreed by both (Claude raised, Codex confirmed with FIX-OK):
Cross-model review by Jarvis 🤖 · head ce4618b · Claude + Codex · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack |
Grants minter rights to the newest marketplace version deployed on the chain rather than always V2, so testnet listings go through V3 while mainnet keeps using V2 until V3 ships there. The authorization modal labels whichever version was resolved. isEnableForSaleOffchain now accepts any deployed version as the minter, not just V1 or V2. Narrowing it to the current version would make every collection listed through an older marketplace look unlisted. The resolution deliberately mirrors decentraland-dapps' getTradeSignature and has to stay in step with it: the marketplace granted minter rights is the one that mints the item, so it must be the version the trade was signed against.
enableSaleOffchain only revoked the newest version, so a collection holding V2 minter rights kept them after the user turned off-chain sale off: listings on that version stayed sellable, and isEnableForSaleOffchain - which accepts any deployed version - still reported the collection as on sale. Enabling still grants the newest version only, since the minter has to be the contract the trade was signed against. Disabling now revokes every version the collection actually holds, falling back to the newest when it holds none so the contract never receives empty arrays.
29.7.1 is the first release where getTradeSignature signs against the newest deployed marketplace rather than V2. Until now Builder granted minter rights to V3 on testnets while dapps still signed the trade against V2, so the authorized minter was not the contract that settles and a primary listing reverted at mint. Adds the cross-package regression test promised on #3472: the address Builder authorizes has to equal the one dapps builds its EIP-712 domain from. Neither repo's own tests can catch that divergence alone. Verified it fails when the two resolution orders disagree.
…red copy Builder authorized the newest marketplace but still signed through src/lib/trades, which hardcoded OffChainMarketplaceV2. On Amoy that granted V3 minter rights while producing a V2 signature domain, so the authorized minter was not the settling contract and a primary listing reverted at mint. The copy existed only because dapps' getValueForTradeAsset had no USD_PEGGED_MANA case; 29.7.1 has it, and its generateTradeValues is byte-identical for both ERC20 and USD-pegged trades, so the file is deleted as its own comment instructed. The regression now asserts the EIP-712 domain the signer is actually handed rather than a resolver that ought to agree with it — comparing resolvers reported this invariant as held while it was broken. Verified it fails when the signer resolves V2.
getLatestOffchainSale repeated dapps' candidate order, so two lists had to stay in step by hand. They agree on every chain today, but this PR already shipped one bug from exactly that kind of drift, and the next marketplace version is when it bites. Builder keeps its own list for a different question: every version a collection might still hold minter rights on, including V1, which the revoke path needs and dapps has no reason to know about. Also drops the resolver-comparison test. With the delegation it compares a value to itself, and a test that cannot fail reads as coverage without being any. The domain assertion stays and still catches a divergent signer - verified.
ce4618b to
449add9
Compare
3 participants
What
Builder grants minter rights to the off-chain marketplace so it can mint the listed item. That was pinned to V2; it now resolves the newest version deployed on the chain, so testnet listings go through V3 while mainnet keeps using V2 until V3 ships there.
enableSaleOffchaingrants the resolved versionPutForSaleOffchainModallabels and authorizes the resolved version instead of hardcoding V2isEnableForSaleOffchainaccepts any deployed version as the minterWhy the check has to widen, not switch
The minter check is what makes a collection show as on sale. If it only accepted the newest version, every collection already listed through V1 or V2 would immediately look unlisted — which is exactly what the V1 → V2 rollout had to handle too (
37531cbbwidened it from V1 to "V1 or V2"). This widens it to "any deployed version", so the next version needs no change here.The ordering constraint that makes this blocked
The marketplace granted minter rights is the one that actually mints, so it has to be the same version the trade was signed against — otherwise the mint reverts. Signing happens in
decentraland-dapps'getTradeSignature, which decentraland/decentraland-dapps#819 moves to the same "newest deployed version" resolution.So this must not merge before a dapps release carrying that change. If Builder granted V3 rights while the shipped dapps still signed V2 trades, listings would break.
The resolution is duplicated here rather than imported from dapps to keep this PR verifiable on its own, and the code says so — the two definitions have to stay in step. Happy to collapse it onto a dapps export as a follow-up once #819 is released.
Testing
src/modules/collection/utils.spec.tspasses with 36 tests, up from 31. New cases cover resolving to V3 on Amoy, reporting V3 as the modal's contract label, falling back to V2 on Polygon mainnet, and a collection minted by V2 still reading as on sale while V3 is current.tscandeslintare clean.I ran every spec that touches the changed helpers, not just this one —
utils.spec.tsis the only one.