Reject interoperable addresses whith both chain reference and addresses empty#6331
Conversation
🦋 Changeset detectedLatest commit: 0e591ba 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 |
WalkthroughThe changes modify the InteroperableAddress contract to validate that inputs are rejected when both chain reference and addresses are empty. The parsing logic in tryParseV1 and tryParseV1Calldata functions is updated so that success is now determined by whether chainReferenceLength or addrLength are non-zero, rather than being unconditionally true. The chainType field is conditionally read only when success is true. Corresponding test vectors are updated to reflect these parsing changes, including a new test case for the empty chain reference and address scenario. A changeset file is added to document the minor release. Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/utils/draft-InteroperableAddress.test.js`:
- Around line 119-125: Fix the typo in the test case description: change "empry
chain reference and address" to "empty chain reference and address" in the test
mapping (the key/value pair where the address string is '0x000100420000') so the
test description reads correctly.
| 'unsupported version': '0x00020000010100', | ||
| // version + ref: missing chainReferenceLength and addressLength | ||
| 'too short (case 1)': '0x00010000', | ||
| 'too short (case 1)': '0x00010042', |
There was a problem hiding this comment.
I don't get why we're changing this test case. With the new code, this is rejected because both chainReferenceLength and addrLength are zero. So the old test still passes: parseV1 reverts and tryParseV1 returns false.
There was a problem hiding this comment.
The current test uses an (invalid) interroperable address with a chainType = 0. When we look at the output of the tryParse, and we see success = false, chainType = 0, we have no way to know if the chain type was parsed (and contains a zero) or if it was not parsed, because the error was detected.
That is why I think we should use a non zero chaintype to make a clear distinction.
| @@ -116,11 +116,13 @@ describe('ERC7390', function () { | |||
| // version 2 + some data | |||
| 'unsupported version': '0x00020000010100', | |||
| // version + ref: missing chainReferenceLength and addressLength | |||
There was a problem hiding this comment.
| // version + ref: missing chainReferenceLength and addressLength | |
| // version + ref: missing chainReferenceLength and address buffer shorter than its declared length |
There was a problem hiding this comment.
In this case there is not address length.
0x00010042....
^^^^ version
^^^^ chain type
^^ expected location of the chainReferenceLength
^^ expected location of the addressLength (if the chainReferenceLength was zero)
There is no chainReferenceLength, and no addressLength. We are not even looking into the chain reference and address buffer
| 'too short (case 1)': '0x00010042', | ||
| // version + ref + chainReference: missing addressLength | ||
| 'too short (case 2)': '0x000100000101', | ||
| 'too short (case 2)': '0x000100420101', |
There was a problem hiding this comment.
Also the old test here was fine
| 'too short (case 2)': '0x000100420101', | ||
| // version + ref + chainReference + addressLength + part of the address: missing 2 bytes of the address | ||
| 'too short (case 3)': '0x00010000010114d8da6bf26964af9d7eed9e03e53415d37aa9', | ||
| 'too short (case 3)': '0x00010042010114d8da6bf26964af9d7eed9e03e53415d37aa9', |
…es empty (#6331) Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
3 participants
Fixes L-06
PR Checklist
npx changeset add)