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

fix(erc20): remove macro export from burnable #74

Closed
wants to merge 1 commit into from

Conversation

qalisander
Copy link
Member

Resolves #73

PR Checklist

  • [-] Tests
  • Documentation

@qalisander qalisander requested review from alexfertel and bidzyyys May 17, 2024 12:31
Copy link

netlify bot commented May 17, 2024

👷 Deploy request for contracts-stylus pending review.

A Netlify team Owner will need to approve the deploy before you can run your build.

Are you a team Owner? Visit the deploys page to approve it

Need more help? Learn more in the Netlify docs

Name Link
🔨 Latest commit ad0c859

@qalisander qalisander force-pushed the fix-erc20-burnable branch from d6afe3f to ad0c859 Compare May 17, 2024 12:32
Copy link

codecov bot commented May 17, 2024

The author of this PR, qalisander, is not an activated member of this organization on Codecov.
Please activate this user on Codecov to display this PR comment.
Coverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations.
Please don't hesitate to email us at [email protected] with any questions.

@qalisander
Copy link
Member Author

I think better not to advertise this approach in public since it pretends that methods will be exported.
In reality it is not.
New tests for burnable example should be added after integration testing pipeline will take a place.

@alexfertel
Copy link
Contributor

Ah, I think @bidzyyys also implemented this change in #71 @qalisander

@bidzyyys
Copy link
Collaborator

That is right, it is refactored & removed in #71

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.

IMHO we should close the PR as it duplicates #71

@qalisander
Copy link
Member Author

Pardon, just noticed. Thought pausable pr will be just for pausable:) But you @bidzyyys can consider this approach create separate example for a burnable erc20 extension (if u think it makes sense ofk)

@qalisander qalisander closed this May 17, 2024
@bidzyyys
Copy link
Collaborator

bidzyyys commented May 18, 2024

Pardon, just noticed. Thought pausable pr will be just for pausable:)

I needed to refactor burnable macros to enable when_not_paused check in burn and burn_from. But I do agree that it is misleading 😅

@alexfertel alexfertel deleted the fix-erc20-burnable branch June 21, 2024 16:49
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.

[Bug]: erc20 burnable doesn't have valid export
3 participants