chore: bump decentraland-transactions to 3.1.1 - #3472
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report for CI Build 33002296984Coverage remained the same at 53.524%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 as part of the coordinated V3 off-chain marketplace rollout with the other linked PRs.
Requesting changes because the Builder stack still does not consume the decentraland-dapps change that makes new trade signatures use the latest deployed off-chain marketplace:
- P1 — Builder grants/recognizes the V3 marketplace through
decentraland-transactions@3.1.1, but it still depends ondecentraland-dapps@^29.3.1-20260805170817.commit-f4ffa6a.createItemOrderTrade()importsgetOffChainMarketplaceContract()andgetTradeSignature()fromdecentraland-dapps/dist/lib/trades; without a Builder dependency bump to the package/version produced by decentraland-dapps#819, Builder can authorize V3 minter rights while signing/reading signature indexes with the old V2-only dapps helper. That violates the cross-PR invariant that the authorized minter and the signature domain must be the same contract.
Please bump Builder's decentraland-dapps dependency once #819 is published (or otherwise wire the same V3-aware helper into Builder) and ideally add a regression test that the minter address selected by Builder matches the contract used by getTradeSignature() on Amoy.
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.
Agreed on the invariant — the authorized minter and the signature domain have to be the same contract — but the bump you're asking for can't land in this PR yet, and deliberately isn't in it.
decentraland-dapps#819 is the change that makes getTradeSignature() sign against the newest deployed marketplace. It is not published yet, so there is no dapps version for Builder to point at. Bumping to the current published range would pin a dapps that still hardcodes V2, which is exactly the mismatch you're describing.
What this PR is for is narrower, and safe on its own: TradeService resolves a trade's contract by address through getContractName(trade.contract), which throws on an unknown address. So the moment the backend records a trade against the V3 address, Builder's accept/cancel fails with Could not get a valid contract name for address 0x36fd… — with no Builder code change needed to trigger it. decentraland-transactions@3.1.1 alone fixes that. It changes no signing and no minter grants.
The V3 listing behaviour, including the dapps bump, is #3473, which is a draft precisely because it is blocked on #819 being released. Sequencing:
- #819 merges and a dapps release goes out
- #3473 bumps
decentraland-dappsand flips minter grants to the resolved version - this PR can merge at any point before or after — it is independent
On the regression test: agreed, and #3473 is the right home for it. It already covers that getLatestOffchainSale() resolves V3 on Amoy and that the modal authorizes the resolved contract, but asserting that address equals the one getTradeSignature() actually signs against is a stronger, cross-package check. I'll add it there once #819 is published and Builder can import the V3-aware helper — until then the assertion can only be written against the old V2-only helper, which would pin the wrong invariant.
LautaroPetaccio
changed the title
TradeService resolves a trade's contract by address through getContractName, which throws on an unknown one. The moment the backend records a trade against the V3 address, accept and cancel fail with "Could not get a valid contract name for address 0x36fd..." — no Builder code change needed to trigger it. The lockfile already resolved 3.1.1 through the old ^3.0.2 range, so this only makes it required rather than incidental. No signing and no minter grants change here.
f6a2941 to
1afa187
Compare
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.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
I re-reviewed the latest diff. This is now dependency-only and safe on its own: it pins the already-resolved decentraland-transactions@^3.1.1 range so trade accept/cancel paths can resolve the new marketplace addresses by contract address. It does not change Builder signing behavior or minter authorization by itself.
No P0/P1/P2 findings.
Security pass: no secrets, auth/authz, input-handling, or direct injection issues found in this PR.
CI status: audit, tests, and Vercel are passing.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
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.
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.
3 participants
What
Bumps
decentraland-transactionsfrom 3.0.2 to 3.1.1, which registers the new off-chain marketplace stack deployed to Amoy and Sepolia:OffChainMarketplaceV3,CouponManagerandCollectionDiscountCoupon.Why this is worth landing on its own
TradeServiceresolves a trade's contract by address, viagetContractName(trade.contract)(decentraland-dapps/dist/modules/trades/TradeService.js:20for accept,:26for cancel), andgetContractNamethrows when the address isn't in the registry. Builder's cancel flow goes straight through it (src/modules/item/sagas.ts:920).So as soon as the backend records a trade against the new marketplace, accepting or cancelling it fails with
Could not get a valid contract name for address 0x36fd…— with no Builder code change needed to trigger it. This bump alone fixes that, which is why it is split out from the V3 migration proper.Scope
Dependency-only:
package.jsonandpackage-lock.json, one package changed. The 3.0.2 → 3.1.1 diff is additive — three new contract files, three registry entries, threeContractNamemembers — with no existing contract entry, address or ABI touched.Switching Builder's listing flow to V3 is separate and depends on decentraland/decentraland-dapps#819.
Why 3.1.1 rather than 3.1.0
3.1.0's root entrypoint re-exported
./crossChain, which imports@0xsquid/sdk— an optional peer dependency since 3.0.0. Builder does not install that peer, andjest.config.tshas no@0xsquidmodule mapper, sorequire('decentraland-transactions')threw:3.0.2 — what Builder pins today — did not have that root re-export; 3.0.3 reintroduced it. decentraland/decentraland-transactions#134 removed it again in 3.1.1, which is what this pins. I verified the root import resolves here with
@0xsquidnot installed, andsrc/modules/collection/utils.spec.tspasses (31 tests).