Call checkOnERC1155BatchReceived when transfering a batch of length 1

[Amxx]

Fixes #6043

Note: if a batch of size one is transfered, this still emits the "TransferSingle" event and not the "TransferBatch" event.

In order to change the event being emited, we would need to add a similar flag to _update ... and have the _updateWithAcceptanceCheck with flag call the _update with flag. This would require changing all the extensions that override _update to perform checks/tasks on all movement (that is the case of the Pausable and Supply extensions). Doing this change would likelly break many user overrides. IMO it should only be done if we do not keep a version without the flag. That would be breaking, so we need to wait for 6.0.

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: a72299f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

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

[coderabbitai]

Walkthrough

This change modifies the ERC1155 token standard implementation to fix callback routing behavior during transfers. The core change introduces an overloaded internal function _updateWithAcceptanceCheck with an additional batch boolean parameter. This parameter distinguishes whether a transfer originated from a batch or single-transfer path. Previously, the code routed to onERC1155Received when array length equaled one, regardless of transfer origin. Now, batch transfers with exactly one token invoke onERC1155BatchReceived with length-one arrays. The changeset documents this as a minor version update, while the CHANGELOG notes this as a breaking change. Tests are expanded to validate correct callback invocations for various transfer scenarios.

Suggested labels

breaking change

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: implementing batch receiver hook calls for single-element batch transfers.
Description check	✅ Passed	The description directly addresses the linked issue and explains the implementation approach, trade-offs, and limitations regarding event emission.
Linked Issues check	✅ Passed	All coding requirements from issue #6043 are met: _updateWithAcceptanceCheck accepts a batch flag parameter, calls route to the correct acceptance hook based on the flag, and batch transfers of length one now invoke checkOnERC1155BatchReceived.
Out of Scope Changes check	✅ Passed	All changes directly support the objective of fixing batch receiver routing; the test additions and internal function overloads are appropriately scoped to the issue requirements.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/token/ERC1155/ERC1155.behavior.js (1)

712-712: Test description doesn't match assertion.

The description says "calls onERC1155Received" but the test expects BatchReceived event. This should be "calls onERC1155BatchReceived" to match the previous similar tests.

-          it('calls onERC1155Received', async function () {
+          it('calls onERC1155BatchReceived', async function () {

🧹 Nitpick comments (1)

test/token/ERC1155/ERC1155.behavior.js (1)

509-515: Duplicate test case.

This test duplicates the logic already covered in lines 478-490 above. Consider removing this block to avoid redundancy.

-      it('reverts when transferring from zero address', async function () {
-        await expect(
-          this.token.$_safeBatchTransferFrom(ethers.ZeroAddress, this.holder, [firstTokenId], [firstTokenValue], '0x'),
-        )
-          .to.be.revertedWithCustomError(this.token, 'ERC1155InvalidSender')
-          .withArgs(ethers.ZeroAddress);
-      });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 79e4988 and 10ac389.

📒 Files selected for processing (5)

• .changeset/vast-worlds-pull.md (1 hunks)
• CHANGELOG.md (1 hunks)
• contracts/token/ERC1155/ERC1155.sol (7 hunks)
• test/token/ERC1155/ERC1155.behavior.js (5 hunks)
• test/token/ERC1155/ERC1155.test.js (2 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-10-15T02:52:05.027Z

Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 5891
File: test/account/modules/ERC7579Module.behavior.js:56-61
Timestamp: 2025-10-15T02:52:05.027Z
Learning: In ERC7579 validator tests for `isValidSignatureWithSender`, using `this.mock` (not bound to a specific account) is valid when testing signature validation with any arbitrary sender, while `this.mockFromAccount` is used when testing account-specific validation scenarios.

Applied to files:

• test/token/ERC1155/ERC1155.test.js
• test/token/ERC1155/ERC1155.behavior.js

📚 Learning: 2025-08-29T13:16:08.640Z

Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.

Applied to files:

• test/token/ERC1155/ERC1155.test.js

🧬 Code graph analysis (2)

test/token/ERC1155/ERC1155.test.js (1)

test/token/ERC1155/ERC1155.behavior.js (5)

• require (1-1)
• require (2-2)
• require (3-3)
• require (5-5)
• require (6-6)

test/token/ERC1155/ERC1155.behavior.js (2)

test/token/ERC1155/extensions/ERC1155Supply.test.js (4)

• firstTokenId (12-12)
• firstTokenValue (13-13)
• secondTokenId (14-14)
• secondTokenValue (15-15)

test/token/ERC1155/extensions/ERC1155Pausable.test.js (4)

• firstTokenId (12-12)
• firstTokenValue (13-13)
• secondTokenId (14-14)
• secondTokenValue (15-15)

🪛 Gitleaks (8.30.0)

test/token/ERC1155/ERC1155.behavior.js

[high] 156-156: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

---

[high] 475-475: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

• GitHub Check: Redirect rules - solidity-contracts
• GitHub Check: Header rules - solidity-contracts
• GitHub Check: Pages changed - solidity-contracts
• GitHub Check: tests-foundry
• GitHub Check: slither
• GitHub Check: tests
• GitHub Check: coverage
• GitHub Check: tests-upgradeable

🔇 Additional comments (9)

CHANGELOG.md (1)

5-5: LGTM!

The breaking change entry clearly documents the behavioral change and correctly indicates that onERC1155BatchReceived will be called with length-one arrays.

.changeset/vast-worlds-pull.md (1)

1-5: Verify changeset version type against breaking change classification.

The CHANGELOG.md classifies this as a "Breaking change", but this changeset uses minor. If this is indeed a breaking behavioral change for existing contracts implementing IERC1155Receiver, consider whether major would be more appropriate per SemVer conventions.

contracts/token/ERC1155/ERC1155.sol (3)

181-193: LGTM!

The backward-compatible overload correctly defaults to ids.length != 1, preserving existing behavior for external callers while allowing internal functions to explicitly control the acceptance check path.

---

204-223: LGTM!

The batch routing logic is correct. When batch=true, it calls checkOnERC1155BatchReceived even for single-element arrays, which aligns with the PR objective of ensuring safeBatchTransferFrom with one element routes to the batch acceptance hook.

---

245-245: LGTM!

All call sites correctly pass the batch flag:

• Single operations (_safeTransferFrom, _mint, _burn) → false
• Batch operations (_safeBatchTransferFrom, _mintBatch, _burnBatch) → true

Also applies to: 272-272, 314-314, 333-333, 351-351, 369-369

test/token/ERC1155/ERC1155.test.js (1)

185-219: LGTM!

Tests comprehensively cover the three key scenarios:

1. Default single-token behavior calls onERC1155Received
2. Explicit batch=true with single token calls onERC1155BatchReceived
3. Multiple tokens call onERC1155BatchReceived

The use of ethers.Typed.bool(true) correctly handles the function overload disambiguation.

test/token/ERC1155/ERC1155.behavior.js (3)

150-166: LGTM!

Good coverage for zero-address sender validation - tests both the public safeTransferFrom (which reverts with ERC1155MissingApprovalForAll) and the internal $_safeTransferFrom (which reverts with ERC1155InvalidSender).

---

534-544: LGTM!

The conditional event emission check correctly handles the new behavior where single-element batch transfers emit TransferSingle instead of TransferBatch.

---

623-693: LGTM!

Good addition of batch size=1 test cases that verify onERC1155BatchReceived is called for single-element batch transfers, aligning with the PR objective.