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

Implement a compliant ERC20 without a name / symbol / decimal functions #27

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sqrlfirst
Copy link

Add an ERC20 without name, symbol, decimals as described in issue #22 from @Magicking

Copy link

@Magicking Magicking left a comment

Choose a reason for hiding this comment

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

Lgtm, added few comments on how I would have done it, great work, thanks for your contribution, I think you would need to chase the repo owner to get your pr accepted

src/ERC20NoViews.sol Outdated Show resolved Hide resolved
Comment on lines 19 to 21
string private constant name = "Token";
string private constant symbol = "TKN";
uint8 private decimals = 18;

Choose a reason for hiding this comment

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

Seems like this could be removed as it's not used anywhere in the contract nor has any possibility to be read from another contract

Copy link
Author

Choose a reason for hiding this comment

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

I think this still can be presented in the contract because the absence of name / symbol / decimal functions may be due to the developer of the contract forgetting to implement them for private variables.

src/ERC20NoViews.sol Outdated Show resolved Hide resolved
@@ -0,0 +1,66 @@
// Copyright (C) 2017, 2018, 2019, 2020, 2024 dbrock, rain, mrchico, d-xo, sqrlfirst

Choose a reason for hiding this comment

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

hey @sqrlfirst I believe that copyright should always go to the owner of the repository!

Choose a reason for hiding this comment

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

Nop, it shall go depending on the CONTRIBUTING.md (which is absent from this repository) and who's doing the code writing... you know the author :)

Choose a reason for hiding this comment

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

hey @Magicking well! I leave it up to the repository owner :)

event Transfer(address indexed src, address indexed dst, uint wad);

// --- Init ---
constructor(uint _totalSupply) public {

Choose a reason for hiding this comment

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

remove public visibility in the constructor!

Choose a reason for hiding this comment

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

Thanks @luislucena16 but if you look line 4, (3 below the copyright), it mentions Solidity 0.6.12 which according to the documentation still mention « public »
See https://docs.soliditylang.org/en/v0.6.12/style-guide.html?#order-of-functions

Choose a reason for hiding this comment

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

yes, of course, but will the old version still be used even for the new features?..

Choose a reason for hiding this comment

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

It's accurate for the solidity compiler version specified, not sure what your words means.

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.

3 participants