fix: wire outfitSlotsAreBetween0and9inclusive into validation chain#352
fix: wire outfitSlotsAreBetween0and9inclusive into validation chain#352LautaroPetaccio merged 5 commits intomainfrom
Conversation
Test this pull request
|
- Add missing outfitSlotsAreBetween0and9inclusiveValidateFn to outfits validation chain — was defined and tested but never called - Fix metadata version timeline lookup to find latest ADR, not oldest - Rename maxThumbnailSizeInB to maxThumbnailDimensionInPx for clarity
- Add integration test: outfits with slot 10 rejected by createOutfitsValidateFn - Add integration test: outfits with valid slots 0 and 4 pass the full chain These tests confirm the newly-wired outfitSlotsAreBetween0and9inclusiveValidateFn is exercised when deployments go through createOutfitsValidateFn.
9e38fe2 to
288ed6f
Compare
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: PR #352 — Wire Outfit Slot Validator, Fix Timeline Lookup, Rename Variable
Verdict: ✅ APPROVE
Three distinct fixes, all correct. The most impactful is wiring the missing slot range validator.
Changes Reviewed
| Fix | Assessment |
|---|---|
Wire outfitSlotsAreBetween0and9inclusiveValidateFn into validateAll |
✅ Critical — validator existed but was never called, allowing invalid slot numbers |
.find() → .filter().pop() in metadata version timeline |
✅ Correct — returns the latest matching ADR instead of the first. Currently latent (1 entry) but prevents future bugs |
Rename maxThumbnailSizeInB → maxThumbnailDimensionInPx |
✅ Correct — value is pixels (1024), not bytes |
Findings
P3 — Nice-to-Have:
-
[Style] The
.filter().pop()pattern works but is slightly less readable thanArray.prototype.findLast(). Since the project targets ES2020 andfindLastis ES2023, the current approach is the right pragmatic choice. Could add afindLastpolyfill toutils.tsif this pattern recurs. -
[Testing] Good integration tests added for the slot validator wiring — both the rejection and acceptance paths are covered. No test exists for the timeline lookup with multiple ADR entries (only one entry exists currently), but that's acceptable since the fix prevents a future bug.
-
[Export] Verify
maxThumbnailDimensionInPxis not exported or referenced externally, since the rename could break consumers. From the diff it appears to be a file-scopedconst, so this is safe.
CI Status
All checks passing. Coverage increased +0.8%.
Requested by Lautaro Petaccio via Slack
The previous expression (thumbnailMaxSizeInMb * 1024) derived a pixel dimension limit from a megabyte constant, which only produced the correct value by coincidence (1 MB * 1024 = 1024). The relationship between file size in megabytes and max pixel dimensions is meaningless. Use a plain literal to make the intent clear.
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review — Focus on Breaking Changes & Lowercasing
⚠️ Breaking Change: Wiring outfitSlotsAreBetween0and9inclusiveValidateFn
This is a new validation being activated. Any existing outfit deployments with slot numbers outside 0-9 will fail validation on redeployment. This is the definition of a breaking change.
Risk assessment: The profile emote validator (emoteUrnsValidateFn in profile.ts) already enforces slot 0-9 for profile emotes. If outfits follow the same convention, existing data should already be in range. However:
- Consider querying the content server for existing outfit entities to confirm no out-of-range slots exist
- Consider wrapping this in a
validateAfterADR_XXXtimestamp gate (matching the pattern used throughout the codebase for introducing new validation rules) so it only applies to new deployments
Timeline Lookup Fix (.find() → .filter().pop())
Good fix. ADRMetadataVersionTimelines is sorted ascending by timestamp. .find() returns the first entry before the timestamp (oldest ADR), while .filter().pop() returns the last (newest/most applicable ADR). Currently the emote timeline has only one entry (ADR 74), so the bug is latent. But this is important to fix before adding a second entry.
No breaking change — behavior is identical with a single timeline entry.
Variable Rename
maxThumbnailSizeInB → maxThumbnailDimensionInPx is a pure documentation improvement. No behavioral change.
Lowercasing
No lowercasing operations in this PR.
Requested by Lautaro Petaccio via Slack
…ions - Add tests for metadataVersionIsCorrectForTimestampValidateFn: valid emoteDataADR74 passes, missing field fails, pre-ADR-74 timestamp skips validation - Restructure outfits integration tests to use describe/beforeEach pattern per dcl testing conventions - Fix duplicate import in outfits.spec.ts
fdf9a0f to
6e1e14f
Compare
Summary
Three fixes for validators that exist in the code but aren't working correctly: a defined-but-never-called validator, a timeline lookup that returns the wrong result, and a misleading variable name.
Outfit slot range validator defined but never wired
outfitSlotsAreBetween0and9inclusiveValidateFnis defined inoutfits.tsand has passing unit tests, but was never added to thecreateOutfitsValidateFnvalidation chain. This means outfits can be deployed with invalid slot numbers (negative, greater than 9, etc.) without triggering a validation error.The function is now inserted into the
validateAll(...)chain afteroutfitSlotsAreNotRepeatedValidateFn, which is the logical position — first check slots aren't repeated, then check they're in range.Metadata version timeline lookup returns oldest ADR instead of latest
metadataVersionIsCorrectForTimestampValidateFnlooks up which ADR version applies to an entity by scanningADRMetadataVersionTimelines. The timeline array is sorted ascending by timestamp, and.find()returns the first match — the oldest ADR before the entity's timestamp. It should return the latest ADR before the timestamp (the most recent version that applies).Today the emote timeline has only one entry (ADR 74), so the bug is latent. But if a second entry is added,
.find()would always return ADR 74 regardless of the entity's timestamp, causing newer emotes to fail metadata version validation.Changed to
.filter(...).pop()which gets the last (newest) matching entry.Misleading variable name
maxThumbnailSizeInBmaxThumbnailSizeInB(declared asthumbnailMaxSizeInMb * 1024 = 1024) is compared againstwidthandheightin pixels, not file bytes. Tests confirm the intended behavior is a 1024-pixel dimension limit. Renamed tomaxThumbnailDimensionInPxto prevent future maintainers from "fixing" the math to* 1024 * 1024.Test plan
npm run buildcompiles cleanlynpm test— all 325 existing tests passnpm run lint:check— no errors🤖 Generated with Claude Code