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: Add address(0) check for addresses in parameters and constructors #96

Closed
wants to merge 4 commits into from

Conversation

lukema95
Copy link

@lukema95 lukema95 commented Nov 4, 2023

Summary

  1. Check if the address in the constructor is address(0)
  2. Check if the address in the parameter is address(0)

Close issue: #92

@andresaiello
Copy link
Collaborator

the changes are ok, but must resolve git conflicts. I can approve it after that

@lukema95
Copy link
Author

lukema95 commented Nov 8, 2023

the changes are ok, but must resolve git conflicts. I can approve it after that

It should be fine now.

@fadeev
Copy link
Member

fadeev commented Nov 8, 2023

@lukema95 thanks so much for your contribution! 🙌 One last request before we merge: please, run yarn generate, so Go files are generated from the updated contracts.

@lukema95
Copy link
Author

lukema95 commented Nov 8, 2023

@lukema95 thanks so much for your contribution! 🙌 One last request before we merge: please, run yarn generate, so Go files are generated from the updated contracts.

@fadeev After running yarn generate, I encountered an error: 'Error: Failed to compile IERC20Permit'. I'm not sure if this is a normal occurrence, as the IERC20Permit contract is an interface contract from the OpenZeppelin library.

@fadeev
Copy link
Member

fadeev commented Nov 8, 2023

@lukema95 created an issue for this: #100

I think it shouldn't prevent merging this PR, since this error exists on main as well.

@fadeev
Copy link
Member

fadeev commented Nov 11, 2023

@andresaiello please, review.

@lumtis
Copy link
Member

lumtis commented Jul 3, 2024

Hey @lukema95 , sorry for the time coming back on this one.
For some context, we started working on new version for the smart contract architecture: #198
As an improvement, it's still beneficial to merge this. So let me know if you want to solve the current conflicts in the PR and we can merge it. Otherwise we can close the issue as the v1 contracts will become deprecated.

@lukema95
Copy link
Author

lukema95 commented Jul 9, 2024

Hey @lukema95 , sorry for the time coming back on this one. For some context, we started working on new version for the smart contract architecture: #198 As an improvement, it's still beneficial to merge this. So let me know if you want to solve the current conflicts in the PR and we can merge it. Otherwise we can close the issue as the v1 contracts will become deprecated.

It doesn't matter.

@lukema95 lukema95 closed this Jul 9, 2024
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.

4 participants