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

Issue #2095 - Provide constants for e.g. the zero address and max uint256 #2109

Merged
merged 15 commits into from
Aug 26, 2021

Conversation

ernestosperanza
Copy link
Contributor

What was wrong?

If I need to compare an address returned from a contract or variable to the zero address or want to e.g. use the maximum allowed uint256, I need to define those myself. (issue #2095)

How was it fixed?

I added the constants.py module and created constants for the following commonly used values:
As strings: ADDRESS_ZERO, MAX_INT, HASH_ZERO
As integer: WEI_PER_ETHER

I replaced the hardcoded values in the codebase with the new constants.
And write a little doc for the constants.

Todo:

If is required I can replace the hardcoded values in docs two, I didn't want to change documentation without permission.

Let me know if there's any improvement that I can apply, thanks :)

@kclowes
Copy link
Collaborator

kclowes commented Aug 19, 2021

Thanks for the PR @ernestosperanza! It looks like the core and ethpm tests are failures that we have in master, but the docs and the lint tests are failures related to this PR. Let me know if you want help fixing those!

@ernestosperanza
Copy link
Contributor Author

@kclowes let me do some documentation reading and follow all the instructions, then if I'm still having problems I will get back to you, thanks

@ernestosperanza
Copy link
Contributor Author

@kclowes I'm still getting errors in the docs but I think that those belong to the ethpm.rst, let me know If I have to modify anything, thanks in advance

@kclowes
Copy link
Collaborator

kclowes commented Aug 25, 2021

I just rebased the current master and pushed. Hopefully that will fix it!

@kclowes
Copy link
Collaborator

kclowes commented Aug 25, 2021

@ernestosperanza - Looks like there are a few real failures where ADDRESS_ZERO needs to be swapped for HASH_ZERO

@ernestosperanza
Copy link
Contributor Author

Sorry, my bad, now should be everything ok.
Let me know if I need to edit any detail, thanks!

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Thanks @ernestosperanza! Looks good, I just left one nitpick!

docs/index.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @ernestosperanza!

@ernestosperanza
Copy link
Contributor Author

Let me know how I can still helping to the proyect, the experience was really enriching for me

@kclowes
Copy link
Collaborator

kclowes commented Aug 26, 2021

Let me know how I can still helping to the proyect

Great! We love contributions! Any of the issues tagged with the "Good First Issue" are usually the best to start with since they don't require a detailed knowledge of the codebase, so keep an eye out for those. We also will need help converting methods to async, but I haven't put any issues up yet. I'll do that soon!

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