Skip to content

Erc1155 implementation#145

Merged
maxnorm merged 31 commits into
Perfect-Abstractions:mainfrom
FrankiePower:ERC1155-Implementation
Oct 31, 2025
Merged

Erc1155 implementation#145
maxnorm merged 31 commits into
Perfect-Abstractions:mainfrom
FrankiePower:ERC1155-Implementation

Conversation

@FrankiePower

@FrankiePower FrankiePower commented Oct 28, 2025 •
edited by maxnorm
Loading

Copy link
Copy Markdown
Contributor

Summary

Changes Made

Checklist

Before submitting this PR, please ensure:

  • Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions, using for directives, or selfdestruct

  • Code follows Design Principles - Readable, uses diamond storage, favors composition over inheritance

  • Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)

  • Code is formatted with forge fmt

  • Tests are included - All new functionality has comprehensive tests

  • All tests pass - Run forge test and ensure everything works

  • Documentation updated - If applicable, update relevant documentation

Make sure to follow the CONTRIBUTING.md guidelines.

Additional Notes

@maxnorm maxnorm self-requested a review October 28, 2025 14:43
@github-actions

github-actions Bot commented Oct 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Coverage Report

Coverage

Metric Coverage Details
Lines 38% 485/1278 lines
Functions 55% 120/217 functions
Branches 21% 51/243 branches

Last updated: Fri, 31 Oct 2025 13:47:18 GMT for commit a885f2a

maxnorm
maxnorm requested changes Oct 28, 2025
View reviewed changes

@maxnorm maxnorm left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! @aapsi @FrankiePower Please check the comment for request changes & fix the formatting errors in the CI.

I think we need to handle the receiver check for minting for built-in security. OZ do it be internally using _updateWithAcceptanceCheck . I asked @mudgen, let's see.

Comment thread src/interfaces/IERC1155.sol Outdated
Comment thread src/interfaces/IERC1155.sol Outdated
Comment thread src/token/ERC1155/LibERC1155.sol
Comment thread src/token/ERC1155/LibERC1155.sol Outdated
@maxnorm maxnorm linked an issue Oct 28, 2025 that may be closed by this pull request
Implement ERC1155 #5
Closed
mudgen and others added 22 commits October 30, 2025 13:12
@mudgen @aapsi
Improved README
0322f9c
@maxnorm @aapsi
add royalty tests
f5ecec5
@maxnorm @aapsi
format test
3f48b3f
@maxnorm @aapsi
adjust to adams feedback
f255107
@Jayy4rl @aapsi
docs: add STYLE.md and update docs to reference coding style guide (c…
2022c17 
…loses Perfect-Abstractions#103)
@mudgen @aapsi
Improved STYLE.md
b01b7ec
@mudgen @aapsi
improved STYLE.md
abe813b
@mudgen @aapsi
improved STYLE.md
5b5db3e
@mudgen @aapsi
improved STYLE.md
d4addff
@mudgen @aapsi
improved STYLE.md
a2b895f
@mudgen @aapsi
improved STYLE.md
16168ce
@mudgen @aapsi
improved STYLE.md
d26996d
@mudgen @aapsi
improved STYLE.md
676ee82
@mudgen @aapsi
improved STYLE.md
c36cd1a
@mudgen @aapsi
Removed unused import statement from ERC721Enumerable
a04dc42
@mudgen @aapsi
improved STYLE.md
d61e5b0
@mudgen @aapsi
Improved STYLE.md
ee146e9
@mudgen @aapsi
Improved STYLE.md
c94836a
@FrankiePower
Merge branch 'Perfect-Abstractions:main' into ERC1155-Implementation
1ec4662
@aapsi
Refactored IERC1155 and IERC1155Receiver interfaces to clarify parame…
23940ea 
…ter descriptions and added IERC1155Receiver interface implementation in a new file.
@FrankiePower
added the transfer functions and erc1155 receiver validation for minting
c1d4639
@FrankiePower
ran forge fmt
35a758b
@FrankiePower FrankiePower requested a review from maxnorm October 30, 2025 12:31
@FrankiePower

FrankiePower commented Oct 30, 2025

Copy link
Copy Markdown
Contributor Author

@maxnorm the tests are included in another PR which will be raised by @aapsi

@FrankiePower FrankiePower marked this pull request as ready for review October 30, 2025 12:46
@FrankiePower FrankiePower requested a review from mudgen October 30, 2025 12:46
maxnorm
maxnorm requested changes Oct 30, 2025
View reviewed changes

@maxnorm maxnorm left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! @FrankiePower & @aapsi . Everything good except a small misunderstanding coming from my last review (check the comment for detail)

Once this is done, we can merge it

Comment thread src/token/ERC1155/LibERC1155.sol Outdated
mudgen
mudgen reviewed Oct 30, 2025
View reviewed changes
Comment thread src/token/ERC1155/ERC1155Facet.sol Outdated
@aapsi

aapsi commented Oct 30, 2025

Copy link
Copy Markdown
Contributor

@FrankiePower Please review the implementation once

@FrankiePower

FrankiePower commented Oct 30, 2025

Copy link
Copy Markdown
Contributor Author

@mudgen @maxnorm i've implemeted the safe transfer requirements and we've resolved the uri handling. review pls

@github-actions

github-actions Bot commented Oct 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Gas Report

No gas usage changes detected between main and ERC1155-Implementation.

All functions maintain the same gas costs. ✅

Last updated: Fri, 31 Oct 2025 13:47:36 GMT for commit a885f2a

maxnorm
maxnorm requested changes Oct 31, 2025
View reviewed changes
Comment thread src/token/ERC1155/LibERC1155.sol Outdated
Comment thread src/token/ERC1155/LibERC1155.sol Outdated
Comment thread src/token/ERC1155/LibERC1155.sol
@aapsi
refactor(ERC1155): rename transfer functions to safeTransferFrom and …
d779165 
…safeBatchTransferFrom, and adjust baseURI handling in ERC1155Storage
@FrankiePower FrankiePower requested a review from maxnorm October 31, 2025 09:12
maxnorm
maxnorm requested changes Oct 31, 2025
View reviewed changes

@maxnorm maxnorm left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work everyone! thanks for the contributions

after this minor change, it's ready for the merge.

Comment thread src/token/ERC1155/LibERC1155.sol
@FrankiePower FrankiePower requested a review from maxnorm October 31, 2025 13:36
@maxnorm

maxnorm commented Oct 31, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@FrankiePower I saw that you had issues with the formatting when running forge fmt

We got similar issues. It was related to the foundry version. Different versions can result to different formatting rules. The CI use 1.4.3-stable for foundry. What i suggest for a quick fix is to upgrade your foundry version, if possible.

In the meantime, i'm currently researching for a fix to avoid mismatch formatting and avoid requiring everyone to upgrade.

maxnorm
maxnorm approved these changes Oct 31, 2025
View reviewed changes

@maxnorm maxnorm left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have nothing else to add. great jobs to both of you @FrankiePower & @aapsi.

as always, thanks for the contribution. we really appreciate your help.

@aapsi, for the extension, let's open new issues for them

@maxnorm maxnorm merged commit 61e301c into Perfect-Abstractions:main Oct 31, 2025
4 checks passed
JackieXu pushed a commit to JackieXu/Compose that referenced this pull request Nov 6, 2025
@maxnorm
Merge pull request Perfect-Abstractions#145 from FrankiePower/ERC1155…
6566723 
…-Implementation

Erc1155 implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maxnorm maxnorm maxnorm approved these changes

@mudgen mudgen Awaiting requested review from mudgen

+1 more reviewer

@aapsi aapsi aapsi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement ERC1155

5 participants

@FrankiePower @aapsi @maxnorm @mudgen @Jayy4rl