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.

Sign up for GitHub

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

Jump to bottom

Add mixins #863

Merged
merged 107 commits into from
Feb 29, 2024
Merged

Add mixins #863

merged 107 commits into from
Feb 29, 2024

Conversation

andrew-fleming
Copy link
Collaborator

@andrew-fleming andrew-fleming commented Jan 2, 2024 •
edited
Loading

Fixes #812.

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

@andrew-fleming
start integrating mixins
fc8c27b
@andrew-fleming
start access control mixin
50fca99
@andrew-fleming
add accesscontrol mixin
bf428a8
@andrew-fleming
fix formatting
5400824
@andrew-fleming
remove test interface
4b62bb5
@andrew-fleming
remove src5 mixin
ec7b064
@andrew-fleming
start account mixins
30158ac
@andrew-fleming
fix mixin names
4a1d67d
@andrew-fleming
fix formatting
7123d79
@andrew-fleming
revert account mixins
df4b309
@andrew-fleming
add erc20 mixins
6d0f5e9
@andrew-fleming
fix formatting
9bdfd85
@andrew-fleming
fix accesscontrol mixin
9b92c15
@andrew-fleming
fix ownable mixin
3611f1b
@andrew-fleming
remove unused interface
e5cebe7
@andrew-fleming
add erc721 mixins
3386f28
@andrew-fleming
fix impl/trait names
2fe3480
@andrew-fleming
fix formatting
927d65e
@andrew-fleming
clean up preset
ccaecab
@andrew-fleming
add account mixin
c2161d4
@andrew-fleming
fix formatting
c69ba5b
@andrew-fleming
finish account mixins
e3c865e
@andrew-fleming
fix formatting
5788077
@andrew-fleming
add src5 to account mixin
ec0b90b
@andrew-fleming
add src5 to erc721 mixin
1b35e5f
@andrew-fleming
add src5 to accesscontrol mixin
b4c63ca
@andrew-fleming
fix formatting
c1ec560
@andrew-fleming
add rec721_receiver mixin
8210763
@andrew-fleming
fix src5 integration
488db5d
@andrew-fleming
fix formatting
d16f6cc
@martriay martriay changed the title Evaluate mixins Add mixins Feb 21, 2024
@martriay martriay mentioned this pull request Feb 21, 2024
Closed
martriay
martriay reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

This is almost ready. I'm very happy with the results 🚀

src/tests/mocks/ownable_mocks.cairo Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/components.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/components.adoc Show resolved Hide resolved
docs/modules/ROOT/pages/components.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/components.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc721.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/account.adoc Outdated Show resolved Hide resolved
ericnordelo
ericnordelo reviewed Feb 23, 2024
View reviewed changes
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

Looking good! Left a couple of comments.

src/presets/account.cairo Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc20.adoc
[.contract-index#ERC20Component-Embeddable-Impls-camelCase]
.Embeddable implementations (camelCase)
--
[.sub-index#ERC20Component-Embeddable-Impls-ERC20CamelOnlyImpl]
Copy link
Member

Choose a reason for hiding this comment

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

Very nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed! this looks great now

docs/modules/ROOT/pages/api/erc721.adoc Outdated
@@ -252,7 +268,7 @@ NOTE: Implementing xref:api/introspection.adoc#SRC5Component[SRC5Component] is a
* xref:#IERC721-Transfer[`++Transfer(from, to, token_id)++`]
--

==== Embeddable functions
==== Embeddable Functions
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? I think it should be lowercase, but if we use uppercase, we should update the rest for consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of the API pages except for ERC721 and ERC20 have Embeddable Functions in title case, so now this heading is at least in a consistent casing. See Access, Account, Introspection, and Security. It's hard to confirm if there are other instances like this haha I can change the embeddable fns back if you prefer lower case

Copy link
Member

Choose a reason for hiding this comment

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

We have "Internal functions" below (in all those cases), so the issue is that we are being inconsistent already. I think we should change "Embeddable Functions" to "Embeddable functions" for consistency with our title casing.

martriay
martriay approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

this looks great! i left a couple comments, but i think it's good to go.
very excited about releasing this one

docs/modules/ROOT/pages/api/erc20.adoc
[.contract-index#ERC20Component-Embeddable-Impls-camelCase]
.Embeddable implementations (camelCase)
--
[.sub-index#ERC20Component-Embeddable-Impls-ERC20CamelOnlyImpl]
Copy link
Contributor

Choose a reason for hiding this comment

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

indeed! this looks great now

docs/modules/ROOT/pages/api/access.adoc Show resolved Hide resolved
docs/modules/ROOT/pages/api/account.adoc
@@ -1,6 +1,7 @@
:github-icon: pass:[<svg class="icon"><use href="#github-icon"/></svg>]
:snip6: https://github.com/ericnordelo/SNIPs/blob/feat/standard-account/SNIPS/snip-6.md[SNIP-6]
:inner-src5: xref:api/introspection.adoc#ISRC5[SRC5 ID]
:mixin-impl: xref:components.adoc#mixins[Embeddable Mixin Implementation]
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if it makes sense to centralize this instead of having it duplicated in each section. we can leave it for later though

docs/modules/ROOT/pages/components.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/components.adoc Outdated Show resolved Hide resolved
ericnordelo
ericnordelo approved these changes Feb 29, 2024
View reviewed changes
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

LGTM!

@martriay martriay merged commit 2e57309 into OpenZeppelin:main Feb 29, 2024
5 checks passed
@martriay martriay mentioned this pull request Mar 1, 2024
Closed
@andrew-fleming andrew-fleming mentioned this pull request Mar 13, 2024
Merged
4 tasks
@andrew-fleming andrew-fleming deleted the add-mixins branch August 6, 2024 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericnordelo ericnordelo ericnordelo approved these changes

@martriay martriay martriay approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Evaluate Mixin impls
3 participants
@andrew-fleming @martriay @ericnordelo