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

feat: typos, removed unsafeBurn, modify unsafeMint #69

Merged
merged 5 commits into from
Nov 28, 2024
Merged

Conversation

jatZama
Copy link
Member

@jatZama jatZama commented Nov 27, 2024

No description provided.

@jatZama jatZama requested review from PacificYield and immortal-tofu and removed request for PacificYield November 27, 2024 14:23
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@PacificYield PacificYield left a comment

Choose a reason for hiding this comment

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

Just one comment about the packageManager

@PacificYield PacificYield self-requested a review November 27, 2024 15:35
@jatZama
Copy link
Member Author

jatZama commented Nov 27, 2024

All tests are working now, so this PR is ready to be merged after review. VERY Important: After my new implementation of Error handling, I noticed one of your test "should be able to transferFrom only if allowance is sufficient" inside EncryptedERC20WithError.test.ts had two logical errors that I fixed. Could you double check that now the correct test logic is indeed implemented please before merging? @PacificYield thanks.
I will let you add later on top of this PR the other features you added, like the new error names etc, after we merge this big PR.

function getTotalNumberErrors() external view returns (uint8 totalNumberErrors) {
return _TOTAL_NUMBER_ERRORS;
function saveError(euint8 errorCode) internal returns (uint256) {
uint256 errorId = _errorCounter;
Copy link
Contributor

Choose a reason for hiding this comment

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

uint256 errorId = _errorCounter++;
You can increment on the same lane.

Copy link
Member Author

@jatZama jatZama Nov 28, 2024

Choose a reason for hiding this comment

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

this reduces readability and doesn't save gas, i dont think it is a good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

OK!

Copy link
Contributor

@PacificYield PacificYield left a comment

Choose a reason for hiding this comment

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

LGTM.

A few suggestions:

  • Start all functions/variables that are internal with underscore (_) https://docs.soliditylang.org/en/latest/style-guide.html#underscore-prefix-for-non-external-functions-and-variables
  • For all functions that deal with errors (from EncryptedErrors), I suggest to start with a recognizable prefix such as "_errorDefineIf", "_errorDefineIfNot". It would help standardizing encrypted error management for future use cases and make it very apparent for developers. --> Can be done in later stages.
  • Make sure to follow the Solidity pattern for visibility of functions or at least be consistent (external / public /internal /private)

@PacificYield
Copy link
Contributor

All tests are working now, so this PR is ready to be merged after review. VERY Important: After my new implementation of Error handling, I noticed one of your test "should be able to transferFrom only if allowance is sufficient" inside EncryptedERC20WithError.test.ts had two logical errors that I fixed. Could you double check that now the correct test logic is indeed implemented please before merging? @PacificYield thanks. I will let you add later on top of this PR the other features you added, like the new error names etc, after we merge this big PR.

I prefer we merge this one and I'll rebase the other PR and continue on this one!

@jatZama
Copy link
Member Author

jatZama commented Nov 28, 2024

All tests are working now, so this PR is ready to be merged after review. VERY Important: After my new implementation of Error handling, I noticed one of your test "should be able to transferFrom only if allowance is sufficient" inside EncryptedERC20WithError.test.ts had two logical errors that I fixed. Could you double check that now the correct test logic is indeed implemented please before merging? @PacificYield thanks. I will let you add later on top of this PR the other features you added, like the new error names etc, after we merge this big PR.

I prefer we merge this one and I'll rebase the other PR and continue on this one!

I agree, this is what I wanted to do exactly.

@jatZama
Copy link
Member Author

jatZama commented Nov 28, 2024

  • Make sure to follow the Solidity pattern for visibility of functions or at least be consistent (external / public /internal /private)

I am not sure to understand what do you mean here? I noticed several points here:
1/ I dunno why you converted all the variables from private to internal. In OZ , almost all variables are private fyi.
2/ Imo the layout and order of functions that you chosed is not very clear. I prefer to use in general this kind of layout to roder functions by their visibility: https://github.com/Cadmos-finance/Stake2Care-sc/blob/master/src/ImpactVault.sol

@jatZama jatZama merged commit 13fafe3 into main Nov 28, 2024
1 check failed
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.

2 participants