Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Hooks _afterTokenTransfer for ERC1155 #3104

Closed
wants to merge 1 commit into from

Conversation

CryptoV8
Copy link
Contributor

Added Hooks of _afterTokenTransfer for ERC1155

Just like ERC721, add the hooks of _afterTokenTransfer for ERC1155.

Currently ERC1155 only have the before Hook, no after hook, hope to add this.

Example

Source Code: https://polygonscan.com/address/0x2048c74851f2ed5c45db70ccb7898a73cfede11e#code#L1343

Merge2048 is ERC-1155, but any address can't hold the same token id.

When the to address receives a token, which to address already had the same id, it'll be merged to the next.

In this case, we need the _afterTokenTransfer to handle it.

@CryptoV8 CryptoV8 changed the title add Hooks _afterTokenTransfer add Hooks _afterTokenTransfer for ERC1155 Jan 12, 2022
@Amxx
Copy link
Collaborator

Amxx commented Jan 12, 2022

Thank you @CryptoV8 for this proposal

@frangio
Copy link
Contributor

frangio commented Jan 13, 2022

I guess this does make sense for simmetry with the other token standards, would you agree @Amxx?

As it is now there is unnecesary memory overhead by invoking _asSingletonArray once for before and once for after, the array can be reused instead.

@Amxx
Copy link
Collaborator

Amxx commented Feb 3, 2022

I'm not able to build on top of this branch, I've added commits and pushed that to my repo:
https://github.com/Amxx/openzeppelin-contracts/tree/feature/ERC1155/afterTokenTransfer

@frangio
Copy link
Contributor

frangio commented Feb 3, 2022

@Amxx It's because @CryptoV8 created this PR from their master branch. You can open a new PR to supercede this one.

@Amxx Amxx mentioned this pull request Feb 3, 2022
3 tasks
@Amxx Amxx closed this Feb 3, 2022
@Amxx
Copy link
Collaborator

Amxx commented Feb 3, 2022

Replaced by #3166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants