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

feat: ERC1155 URIStorage Extension #431

Merged
merged 19 commits into from
Dec 6, 2024
Merged

feat: ERC1155 URIStorage Extension #431

merged 19 commits into from
Dec 6, 2024

Conversation

0xNeshi
Copy link
Collaborator

@0xNeshi 0xNeshi commented Dec 3, 2024

Since Erc1155-related benches and tests have become huge, decided to split them into separate files/directories.

Resolves #351

PR Checklist

  • Tests
  • Documentation
  • Changelog

Copy link

netlify bot commented Dec 3, 2024

Deploy Preview for contracts-stylus ready!

Name Link
🔨 Latest commit 34e0de4
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/6751f8d01e7b5e0008e6cace
😎 Deploy Preview https://deploy-preview-431--contracts-stylus.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.


sol!(
#[sol(rpc)]
contract Erc1155 {
Copy link
Collaborator Author

@0xNeshi 0xNeshi Dec 3, 2024

Choose a reason for hiding this comment

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

Only the subset of functions actually used in tests is sufficient to be defined here, but I noticed that some other contract examples have the full set even though that's not necessary.

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 72.00000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 64.2%. Comparing base (504b083) to head (34e0de4).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
...tracts/src/token/erc1155/extensions/uri_storage.rs 72.0% 14 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
...tracts/src/token/erc1155/extensions/uri_storage.rs 72.0% <72.0%> (ø)

@bidzyyys
Copy link
Collaborator

bidzyyys commented Dec 3, 2024

TBH I am not a fan of having so many examples...
Our repository is becoming huge. I would say make it in a single example when it is possible - when there is no functions that cannot be tested while having all the extensions added. Wdyt @qalisander?

Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

Good job @0xNeshi, left major comments after first review.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc1155-uri-storage.adoc Outdated Show resolved Hide resolved
}

#[public]
#[inherit(Erc1155, Erc1155MetadataUri)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO you do not need to inherit Erc1155MetadataUri as you overwrote uri.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@qalisander @bidzyyys does it make sense to do impl IErc1155MetadataUri for Erc1155UriStorage? 🤔
It does technically extend (and overwrite) its uri function

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think so

Copy link
Member

Choose a reason for hiding this comment

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

p.s. @0xNeshi also we don't need #[borrow] for Erc1155MetadataUri in this case

examples/erc1155-metadata-uri/src/lib.rs Show resolved Hide resolved
@bidzyyys bidzyyys requested a review from ggonzalez94 December 3, 2024 12:46
@0xNeshi
Copy link
Collaborator Author

0xNeshi commented Dec 4, 2024

TBH I am not a fan of having so many examples... Our repository is becoming huge. I would say make it in a single example when it is possible - when there is no functions that cannot be tested while having all the extensions added. Wdyt @qalisander?

This concern was addressed in a private group, copy/pasting the explanation for an additional example, just for future reference:

That's exactly what happened when I tried running the unified Erc1155 + Metadata+ Uri storage test - it exceeded the max contract size.

@0xNeshi 0xNeshi requested a review from bidzyyys December 4, 2024 09:00
Cargo.toml Show resolved Hide resolved
Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

Left last comments.

Copy link
Member

@qalisander qalisander left a comment

Choose a reason for hiding this comment

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

LGTM!

@0xNeshi 0xNeshi requested a review from bidzyyys December 5, 2024 18:59
Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

LGTM!

@0xNeshi 0xNeshi merged commit 60bb984 into main Dec 6, 2024
24 checks passed
@0xNeshi 0xNeshi deleted the erc1155-uri-storage branch December 6, 2024 19:03
0xNeshi added a commit that referenced this pull request Dec 9, 2024
feat: add erc1155-uri-storage

docs: add docs

test: move erc1155 metadata-related tests to separate dir

test: add metadata-uri + uri-storage tests

chore: add benches

test: add additional unit tests + rename some e2e tests

ref: simplify erc1155-metadata-uri example

docs: simplify erc1155-metadata-uri example in adoc

ref: remove baseUri_ ctor param + add setBaseURI fn to tests + bench

ref: remove 'pub' from erc1155 example/../lib.rs

docs: update CHANGELOG

revert change to e2e script

ref: make TOKEN_URI a const in bench

ref: remove unnecessary let binding in 'url'

docs: moved the added item to the top of the list in CHANGELOG

test: remove Erc1155MetadataUri from inherit in example

docs: simplify example in adoc

remove useless #[borrow]

docs: remove borrow attr from metadata_uri in adoc
0xNeshi added a commit that referenced this pull request Dec 9, 2024
feat: add erc1155-uri-storage

docs: add docs

test: move erc1155 metadata-related tests to separate dir

test: add metadata-uri + uri-storage tests

chore: add benches

test: add additional unit tests + rename some e2e tests

ref: simplify erc1155-metadata-uri example

docs: simplify erc1155-metadata-uri example in adoc

ref: remove baseUri_ ctor param + add setBaseURI fn to tests + bench

ref: remove 'pub' from erc1155 example/../lib.rs

docs: update CHANGELOG

revert change to e2e script

ref: make TOKEN_URI a const in bench

ref: remove unnecessary let binding in 'url'

docs: moved the added item to the top of the list in CHANGELOG

test: remove Erc1155MetadataUri from inherit in example

docs: simplify example in adoc

remove useless #[borrow]

docs: remove borrow attr from metadata_uri in adoc
0xNeshi added a commit that referenced this pull request Dec 9, 2024
feat: add erc1155-uri-storage

docs: add docs

test: move erc1155 metadata-related tests to separate dir

test: add metadata-uri + uri-storage tests

chore: add benches

test: add additional unit tests + rename some e2e tests

ref: simplify erc1155-metadata-uri example

docs: simplify erc1155-metadata-uri example in adoc

ref: remove baseUri_ ctor param + add setBaseURI fn to tests + bench

ref: remove 'pub' from erc1155 example/../lib.rs

docs: update CHANGELOG

revert change to e2e script

ref: make TOKEN_URI a const in bench

ref: remove unnecessary let binding in 'url'

docs: moved the added item to the top of the list in CHANGELOG

test: remove Erc1155MetadataUri from inherit in example

docs: simplify example in adoc

remove useless #[borrow]

docs: remove borrow attr from metadata_uri in adoc
0xNeshi added a commit that referenced this pull request Dec 9, 2024
<!--
Thank you for your interest in contributing to OpenZeppelin!

Consider opening an issue for discussion prior to submitting a PR. New features will be merged faster if they were first discussed and designed with the team.

Describe the changes introduced in this pull request. Include any context necessary for understanding the PR's purpose.
-->

Since Erc1155-related benches and tests have become huge, decided to split them into separate files/directories.

<!-- Fill in with issue number -->
Resolves #351

<!--
Before merging the pull request all of the following must be completed.
Feel free to submit a PR or Draft PR even if some items are pending.
Some of the items may not apply.
-->

- [x] Tests
- [x] Documentation
- [x] Changelog

---------

Co-authored-by: Alisander Qoshqosh <[email protected]>
0xNeshi added a commit that referenced this pull request Dec 9, 2024
<!--
Thank you for your interest in contributing to OpenZeppelin!

Consider opening an issue for discussion prior to submitting a PR. New features will be merged faster if they were first discussed and designed with the team.

Describe the changes introduced in this pull request. Include any context necessary for understanding the PR's purpose.
-->

Since Erc1155-related benches and tests have become huge, decided to split them into separate files/directories.

<!-- Fill in with issue number -->
Resolves #351

\#### PR Checklist

<!--
Before merging the pull request all of the following must be completed.
Feel free to submit a PR or Draft PR even if some items are pending.
Some of the items may not apply.
-->

- [x] Tests
- [x] Documentation
- [x] Changelog

---------

Co-authored-by: Alisander Qoshqosh <[email protected]>
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.

[Feature]: ERC1155 URIStorage Extension
3 participants