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

Migrate ERC20 to component #780

Merged
merged 83 commits into from
Nov 2, 2023
Merged

Migrate ERC20 to component #780

merged 83 commits into from
Nov 2, 2023

Conversation

andrew-fleming
Copy link
Collaborator

@andrew-fleming andrew-fleming commented Oct 9, 2023 •
edited
Loading

Fixes #779. Doc update forthcoming.

This PR also proposes to include pop_log_comp_indexed to extract event args in component events with indexed params.

PR Checklist

  • Tests
  • Tried the feature on a public network
  • Documentation

@andrew-fleming
update Scarb
26bfa51
@andrew-fleming
remove attrs
a78dd60
@andrew-fleming
fix formatting
8d433ab
@andrew-fleming
add erc20_mocks
913f93f
@andrew-fleming
add interface attr
b0526f3
@andrew-fleming
migrate erc20 to component
81bb55e
@andrew-fleming
import erc20 contract
80f53dc
@andrew-fleming
change erc20 import
ab59f7b
@andrew-fleming
update tests, comment out event assertions
da90751
@andrew-fleming
add pop_log_comp_indexed
19bb8c4
@andrew-fleming
remove line
d1399db
@andrew-fleming
remove unused mocks
6d31695
@andrew-fleming
fix formatting
7a4873d
@andrew-fleming
start erc20 doc edit for components
8d071a3
@andrew-fleming
change _comp to _component
d4dee83
@andrew-fleming
simplify InternalImpl with generate_trait
a2fcf23
@andrew-fleming
fix conflicts
c5e1087
@andrew-fleming
add safe allowance interfaces
40e553c
@andrew-fleming
fix interface comments
8337a0b
@andrew-fleming
add impls to API index
8e719d5
@andrew-fleming
fix ERC20ABI
d5f35df
@andrew-fleming
fix compatibility section
e7c59e7
@andrew-fleming
update usage section
5c8d1e5
@andrew-fleming
add erc20metadata
ec80861
@andrew-fleming
add erc20metadata to usage section
ba7505c
@andrew-fleming
fix formatting
8f7dac4
@andrew-fleming
update decimals guide
d88a564
@andrew-fleming
update supply guide
d95c1a0
@andrew-fleming
integrate metadata to api
e53c7b0
@andrew-fleming
fix InternalImpl in snippets
77d87e3
martriay
martriay reviewed Oct 27, 2023
View reviewed changes
src/token/erc20/interface.cairo Show resolved Hide resolved
docs/modules/ROOT/pages/guides/erc20-supply.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/guides/erc20-supply.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc20.adoc Outdated Show resolved Hide resolved
martriay
martriay reviewed Oct 27, 2023
View reviewed changes
docs/modules/ROOT/pages/api/erc20.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/erc20.adoc Outdated Show resolved Hide resolved
martriay
martriay reviewed Oct 28, 2023
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.

We're very close! We're just missing:

  • update README example
  • update v0.7.0 references to v0.8.0-beta.0

docs/modules/ROOT/pages/api/erc20.adoc Outdated Show resolved Hide resolved
ericnordelo
ericnordelo approved these changes Oct 30, 2023
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.

Looks ready to me!

martriay
martriay approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@martriay martriay left a comment •
edited
Loading

Choose a reason for hiding this comment

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

good to go! left a final suggestion for the readme. it makes me wonder whether we should also update the overview section to use ERC20 as it is simpler, and probably more used than the current Account example. edit: #805

README.md Outdated Show resolved Hide resolved
@andrew-fleming
Copy link
Collaborator Author

I added the Component suffix and changed Functions => Implementations in the erc20 API. I can move the changes into a separate PR if preferred @martriay @ericnordelo

ericnordelo
ericnordelo approved these changes Nov 1, 2023
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 approved these changes Nov 1, 2023
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.

LGTM, let's fix conflicts and merge 🚀

@martriay martriay merged commit 841a073 into OpenZeppelin:main Nov 2, 2023
1 check passed
@andrew-fleming andrew-fleming deleted the component-erc20 branch November 7, 2023 22:32
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.

Migrate ERC20 to component
3 participants
@andrew-fleming @ericnordelo @martriay