dn404 icon indicating copy to clipboard operation
dn404 copied to clipboard

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

Open codebuster22 opened this issue 1 year ago • 4 comments

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:

  • [x] Ran forge fmt?
  • [x] Ran forge snapshot?
  • [x] Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

codebuster22 avatar Feb 14 '24 11:02 codebuster22

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.

Vectorized avatar Feb 14 '24 11:02 Vectorized

Got it, I'll work on it.

Thanks for the prompt review.

codebuster22 avatar Feb 14 '24 12:02 codebuster22

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.

  • Is it because of any side effect from the change propose in this PR?
  • Just to confirm _MAX_SUPPLY represents the maximum number of ERC20 tokens that can exist?

codebuster22 avatar Feb 14 '24 12:02 codebuster22

#63

Vectorized avatar Feb 15 '24 00:02 Vectorized

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

Vectorized avatar Feb 15 '24 04:02 Vectorized

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?

codebuster22 avatar Feb 15 '24 05:02 codebuster22

@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.

Vectorized avatar Feb 16 '24 23:02 Vectorized