Add an optional parameters to the InteroperableAddress parssing functions#6481
Add an optional parameters to the InteroperableAddress parssing functions#6481Amxx wants to merge 3 commits intoOpenZeppelin:masterfrom
Conversation
🦋 Changeset detectedLatest commit: 5c11cc8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change introduces optional Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.changeset/tall-aliens-matter.md (1)
5-5: Clarify default behavior in the changeset note.Consider explicitly stating that trailing bytes are still ignored by default for backward compatibility, and only rejected when
failOnExtraBytes=true.✍️ Suggested wording
-`InteroperableAddress`: Add an optional `failOnExtraBytes` parameter to all parsing functions. If enabled, the parsing will reject any input that contains extra bytes. +`InteroperableAddress`: Add an optional `failOnExtraBytes` parameter to all parsing functions. By default (`false`), trailing bytes are ignored for backward compatibility; when set to `true`, parsing rejects inputs with extra trailing bytes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/tall-aliens-matter.md at line 5, Update the changeset entry for InteroperableAddress to explicitly state that all parsing functions gain an optional failOnExtraBytes parameter which defaults to false for backward compatibility, meaning trailing/extra bytes are ignored by default; clarify that only when failOnExtraBytes=true will parsing reject inputs containing extra bytes. Mention the default behavior and the opt-in nature of failOnExtraBytes so consumers understand compatibility and the new strict mode.test/utils/draft-InteroperableAddress.test.js (1)
71-207: Consider extracting helper assertions to reduce repetition.The new cases are correct, but this block is quite repetitive; a small helper per API family would make future updates safer and easier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/draft-InteroperableAddress.test.js` around lines 71 - 207, Tests repeat the same assertion patterns for the V1 and EVM families; extract small helpers to reduce duplication: create reusable assertion helpers (e.g., assertParseFamily(mock, fnNamesArray, input, ignoreTrailing?, expected) and assertTryParseFamily(...)) and use them for [$parseV1, $parseV1Calldata, $tryParseV1, $tryParseV1Calldata] and for the EVM variants [$parseEvmV1, $parseEvmV1Calldata, $tryParseEvmV1, $tryParseEvmV1Calldata, $formatEvmV1], parameterizing the input (binary, binaryExtra), the ignoreTrailing flag (ethers.Typed.bool(true/false)), and expected outputs (expected, [true,...expected], error cases), then replace the duplicated await expect(...) blocks with calls to these helpers referencing symbols like this.mock, binaryExtra, expected, reference, address, and ethers.Typed.bool to keep behavior identical while removing repetition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/tall-aliens-matter.md:
- Line 5: Update the changeset entry for InteroperableAddress to explicitly
state that all parsing functions gain an optional failOnExtraBytes parameter
which defaults to false for backward compatibility, meaning trailing/extra bytes
are ignored by default; clarify that only when failOnExtraBytes=true will
parsing reject inputs containing extra bytes. Mention the default behavior and
the opt-in nature of failOnExtraBytes so consumers understand compatibility and
the new strict mode.
In `@test/utils/draft-InteroperableAddress.test.js`:
- Around line 71-207: Tests repeat the same assertion patterns for the V1 and
EVM families; extract small helpers to reduce duplication: create reusable
assertion helpers (e.g., assertParseFamily(mock, fnNamesArray, input,
ignoreTrailing?, expected) and assertTryParseFamily(...)) and use them for
[$parseV1, $parseV1Calldata, $tryParseV1, $tryParseV1Calldata] and for the EVM
variants [$parseEvmV1, $parseEvmV1Calldata, $tryParseEvmV1,
$tryParseEvmV1Calldata, $formatEvmV1], parameterizing the input (binary,
binaryExtra), the ignoreTrailing flag (ethers.Typed.bool(true/false)), and
expected outputs (expected, [true,...expected], error cases), then replace the
duplicated await expect(...) blocks with calls to these helpers referencing
symbols like this.mock, binaryExtra, expected, reference, address, and
ethers.Typed.bool to keep behavior identical while removing repetition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4bd56e3d-9c92-4b42-9d90-cd3847283679
📒 Files selected for processing (3)
.changeset/tall-aliens-matter.mdcontracts/utils/draft-InteroperableAddress.soltest/utils/draft-InteroperableAddress.test.js
MycCellium420
left a comment
There was a problem hiding this comment.
Clean approach — overloading with defaults keeps backward compatibility intact while giving callers strict validation when needed.
A couple of minor things:
Title typo: "parssing" → "parsing"
Nit: The expectedLength extraction in tryParseV1 / tryParseV1Calldata is a nice cleanup — reduces the repeated 0x06 + chainReferenceLength + addrLength calculation even for the non-strict path.
Docs: Checkbox is unchecked — is documentation planned for a follow-up?
Tests look thorough — covers default/explicit-false/true for all memory and calldata variants. LGTM otherwise.
Requested by MatterLabs
PR Checklist
npx changeset add)