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

Add ERC20 example #995

Merged
merged 2 commits into from Aug 16, 2018
Merged

Add ERC20 example #995

merged 2 commits into from Aug 16, 2018

Conversation

njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented Aug 8, 2018

What was wrong?

No ERC20 example in docs.

fixes #951

How was it fixed?

Added an example showing how to interact with an erc20 contract.

Cute Animal Picture

image

@njgheorghita
Copy link
Contributor Author

Should the ethpm example eventually go here? or under the pm-module doc page?

@njgheorghita njgheorghita force-pushed the erc20-documentation branch 2 times, most recently from 22285a7 to aa951fc Compare August 8, 2018 19:11
Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

Once eth-pm is on a stable release, then it can go in the general examples page. Until then, it should probably stay in the eth-pm-specific one.

.. code-block:: python

name = erc20.functions.symbol().name()
# -> 🦄
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name should be Unicorns


.. code-block:: python

count = balance / (10 ** decimals)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try Decimal() instead of float here, with a high enough precision to avoid any rounding error. Most applications expect exact results, even if fractional.

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

Finger slip, meant to explicitly request the Decimal thing to be resolved.


Fetch various data about the current state of the ERC20 contract.

.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

if we're willing to do this over an infura HTTP provider these could actually be doctested quite easily

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

Would be cool to get doctest against Infura working.


Balance is *always* an integer in the currency's smallest natural unit (i.e. `wei` for ether). Token balance is typically used only for backend calculations.

Token count *may* be a integer or fraction in the currency's primary unit (i.e. `eth` for ether). Token count is typically displayed to the user on the front-end since it is more readable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of i.e. (which means "in other words"). Since ether isn't an ERC20 token, this could be confusing. I would say something like (equivalent to 'eth' for ether)

@njgheorghita njgheorghita force-pushed the erc20-documentation branch 3 times, most recently from aab8d43 to 46b0cc4 Compare August 14, 2018 19:35
@njgheorghita
Copy link
Contributor Author

@carver I added doctest to the erc20 example, not sure if there's a better solution for using the Infura API key? Open to any suggestions or leaving as is.

@carver
Copy link
Collaborator

carver commented Aug 15, 2018

@njgheorghita Yeah, I assume you generated a fresh API key for this?

Yeah, the key will have to be exposed somehow, in the source. But at least we can keep it from showing in the docs, by moving it to a test setup block: http://www.sphinx-doc.org/en/master/usage/extensions/doctest.html#directive-testsetup

@pipermerriam
Copy link
Member

@njgheorghita you could also use an encrypted environment variable on circle-ci but that won't really save us since we allow CI to run on forks and thus, anyone can extract the environment variable simply by exporting it with custom code during a CI run....

>>> import os

# Please play nice and don't use this key for any shenanigans, thanks!
>>> os.environ['INFURA_API_KEY'] = "b2679bb624354d1b9a2586154651b51f"
Copy link
Member

Choose a reason for hiding this comment

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

👍 to @carver suggestion to move this into a setup block so it doesn't show up in the actual documentation.

@njgheorghita
Copy link
Contributor Author

@carver Yup, generated a fresh key for this, happy to keep an eye on it for any abuse and regenerate if needed. Thanks for the setup tip, moved the key to a hidden block. Let me know if you'd like any other changes

@carver
Copy link
Collaborator

carver commented Aug 16, 2018

LGTM

@njgheorghita
Copy link
Contributor Author

@carver 👍 If you're waiting on me, I don't have write access so pull the trigger anytime

@carver carver merged commit 61cc3b1 into ethereum:master Aug 16, 2018
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.

Add an ERC20 balance check example to the docs
3 participants