Vectorized / dn404

Implementation of a co-joined ERC20 and ERC721 pair.
MIT License
476 stars 158 forks source link

✨ Support custom WAD to allow devs to specific custom base units #60

Closed codebuster22 closed 9 months ago

codebuster22 commented 9 months ago

Description

Convert constant _WAD variable to _WAD internal and virtual function to support custom Base unit (_WAD) by overriding _WAD function. Reference #59

Checklist

Ensure you completed all of the steps below before submitting your pull request:

Pull requests with an incomplete checklist will be thrown out.

Vectorized commented 9 months ago

I will review this later.

Note that the internal _MAX_SUPPLY must be changed into an internal function.

Add checks in the initializer function to ensure decimals() and _WAD() are overridden properly. And _MAX_SUPPLY must not exceed 2**96 - 1.

codebuster22 commented 9 months ago

Got it, I'll work on it.

Thanks for the prompt review.

codebuster22 commented 9 months ago

Few Questions:

Add checks in the initializer function to ensure decimals() and _WAD() are overridden properly.

I understand the need to check _WAD(), but currently the decimals function cannot be overridden as it's not marked virtual. To allow overriding we will have to mark decimals() as virtual. I consider this to be a different PR to ensure separation of concerns. I'm open to your feedback.

Note that the internal _MAX_SUPPLY must be changed into an internal function.

Vectorized commented 9 months ago

63

Vectorized commented 9 months ago

Thank you so much for the good issue and code samples. Will be closing this.

codebuster22 commented 9 months ago

I'm happy it was helpful.

Just from a feedback perspective: How could I have improved my PR or code so that the changes could have been used as it is?

Vectorized commented 9 months ago

@codebuster22

Don't worry about it, it's just that the development cadence is quite fast now.

I was simply unsure how the code will look like and quickly wrote some stuff and tests on another branch.