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/dirtycheck #73

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ registrar.json

# Crowdsale definitions
crowdsales/*.yml
crowdsales/*.py
*.csv

# Customer test cases
Expand Down
4 changes: 3 additions & 1 deletion ico/cmd/deploycontracts.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from populus import Project

from ico.deploy import deploy_crowdsale_from_file

from ico.git import git_sanitycheck

@click.command()
@click.option('--deployment-name', nargs=1, default="mainnet", help='YAML section name we are deploying. Usual options include "mainnet" or "kovan"', required=True)
Expand All @@ -27,6 +27,8 @@ def main(deployment_file, deployment_name, address):
* https://github.com/TokenMarketNet/ico/blob/master/crowdsales/example.yml
"""

git_sanitycheck()

project = Project()
deploy_crowdsale_from_file(project, deployment_file, deployment_name, address)
print("All done! Enjoy your decentralized future.")
Expand Down
7 changes: 4 additions & 3 deletions ico/cmd/distributetokens.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
@click.option('--issuer-address', nargs=1, help='The address of the issuer contract - leave out for the first run to deploy a new issuer contract', required=False, default=None)
@click.option('--master-address', nargs=1, help='The team multisig wallet address that does StandardToken.approve() for the issuer contract', required=False, default=None)
@click.option('--allow-zero/--no-allow-zero', default=False, help='Stops the script if a zero amount row is encountered')
def main(chain, address, token, csv_file, limit, start_from, issuer_address, address_column, amount_column, allow_zero, master_address):
@click.option('--gas-price', nargs=1, help='Gas price for a transaction in Gweis', default=30)
def main(chain, address, token, csv_file, limit, start_from, issuer_address, address_column, amount_column, allow_zero, master_address, gas_price):
"""Distribute tokens to centrally issued crowdsale participant or bounty program participants.

Reads in distribution data as CSV. Then uses Issuer contract to distribute tokens.
Expand Down Expand Up @@ -77,7 +78,7 @@ def main(chain, address, token, csv_file, limit, start_from, issuer_address, add

decimal_multiplier = 10**decimals

transaction = {"from": address}
transaction = {"from": address, "gasPrice": gas_price * 1000000000}

Issuer = c.provider.get_base_contract_factory('Issuer')
if not issuer_address:
Expand Down Expand Up @@ -170,7 +171,7 @@ def main(chain, address, token, csv_file, limit, start_from, issuer_address, add

transaction = {
"from": address,
"gasPrice": int(web3.eth.gasPrice * 1.5)
"gasPrice": gas_price * 1000000000
Copy link

Choose a reason for hiding this comment

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

yep - it all seems good. - ztane have placed some comments already.

But these numbers with a lot of zeros are really hard to read correctly, and they are very critical - I'd suggest creating a global constant of one million - ie - a line in the beggining of the file like MILLION = int(10e6)

and them write these gas_price expressions as: gasprice * 100 * MILLION
( int(10e8) would work, of course, but I find 100 * MILLION more readable)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have totally different view on this, even though it's a rather small matter. I would say having constant like MILLION = int(10e6) is not only not correct (10e6 is actually 10**7 => 10 millions) but also doesn't really improve the readability of the code.

The context of this part of code is to convert gas_price from wei to gigawei. So the thing that developers really care about is how many 0 there are, which is the reason why we can say 1000000000 is a hard number to read => easy to make mistake. And for that reason alone it's best to use exponentiation.

And if you want to take it a little bit further, then you can define an utility function like:

def wei_to_gwei(price):
    return price * 10 ** 9  # or price * (10 ** 9) if you don't trust your memory on operator precedence 

then it's readable, understandable, reusable and testable.

Copy link

Choose a reason for hiding this comment

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

I would not say it is small matter - and neither that it is "totally different" view. The major problem, of course is reading the zeros. I agree that putting the exponentiation directly will make the number of zeros more apparent (and less prone to subtle catastrophic mistakes as the one I just made, of writting 10e6 thinking about 10 ** 6)

Copy link

Choose a reason for hiding this comment

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

There is the other matter that int(1e6) is not a compile-time constant, unlike 10 ** 6.

}

tokens = int(tokens)
Expand Down
2 changes: 0 additions & 2 deletions ico/deploy.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,5 +286,3 @@ def deploy_crowdsale_from_file(project: Project, yaml_filename: str, deployment_
with project.get_chain(chain_name) as chain:
web3 = chain.web3
return _deploy_contracts(project, chain, web3, yaml_filename, chain_data, deploy_address)


2 changes: 1 addition & 1 deletion ico/etherscan.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def _fill_in_textarea_value(browser, splinter_elem, value):



def verify_contract(project: Project, chain_name: str, address: str, contract_name, contract_filename: str, constructor_args: str, libraries: dict, optimization=True, optimizer_runs=200, compiler: str="v0.4.8+commit.60cc1668", browser_driver="chrome") -> str:
def verify_contract(project: Project, chain_name: str, address: str, contract_name, contract_filename: str, constructor_args: str, libraries: dict, optimization=True, optimizer_runs=200, compiler: str="v0.4.15+commit.bbb8e64f", browser_driver="chrome") -> str:
"""Semi-automated contract verified on Etherscan.

Uses a web browser + Selenium auto fill to verify contracts.
Expand Down
41 changes: 41 additions & 0 deletions ico/git.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
from git import Repo
import os
import time

join = os.path.join
repo = Repo(os.getcwd())
assert not repo.bare

repo.config_reader()

def git_sanitycheck():
git_ismaster()
git_isold()
git_isdirty()

def git_isdirty():
if repo.is_dirty():
raise RuntimeError("The repository is not committed, won't continue. Please commit.")

return

def git_ismaster():
# User can override git_isold checking for a week
if ((float(os.getenv("ICO_DEPLOY_ANYWAY", 0)) + 604800) > time.time()):
Copy link
Contributor

Choose a reason for hiding this comment

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

As @ztane mentioned, you don't have to use that many parentheses in Python if, here it's perfectly fine to do this:

if float(os.getenv("ICO_DEPLOY_ANYWAY", 0)) + 604800 > time.time():

And if it's me, I would probably move float(os.getenv("ICO_DEPLOY_ANYWAY", 0)) + 604800 to a separate variable before this condition just to make it a little bit easier to understand, something like this:

deploy_anyway = os.getenv("ICO_DEPLOY_ANYWAY", 0)  # actually I don't know what this value, should it contain which type?
one_week = 60 * 60 * 24 * 7
now = time.time()

if deploy_anyway + one_week > now:

Btw why is this time check done in this function and not in git_isold()? It doesn't seem to relate to the current function that much.

return True

if (repo.active_branch.commit != repo.heads.master.commit):
raise RuntimeError("This branch is not 'master'. Please switch to master, or use the following command to postpone this check for a week:\nexport ICO_DEPLOY_ANYWAY=" + str(time.time()))

def git_isold():
git_root = repo.git.rev_parse("--show-toplevel")
latest_pull = os.stat(git_root + "/.git/FETCH_HEAD").st_mtime
deadline = latest_pull + 604800 # One week from latest commit

if (time.time() > deadline):
raise RuntimeError("You haven't pulled for a week. Please do git pull.")
else:
return

def git_current_commit():
return repo.active_branch.commit
5 changes: 4 additions & 1 deletion ico/importexpand.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Mainly need for EtherScan verification service.
"""
import os
from ico.git import git_current_commit
from typing import Tuple

from populus import Project
Expand Down Expand Up @@ -83,4 +84,6 @@ def expand_contract_imports(project: Project, contract_filename: str) -> Tuple[s
:return: Tuple[final expanded source, set of processed filenames]
"""
exp = Expander(project)
return exp.expand_file(contract_filename), exp.processed_imports
commitline = "// (C) 2017 TokenMarket Ltd. (https://github.com/TokenMarketNet/ico/blob/master/LICENSE.txt) Commit: " + str(git_current_commit()) + "\n"

return commitline + exp.expand_file(contract_filename), exp.processed_imports
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ eth-testrpc==1.3.0
ethereum==1.6.1
ethereum-abi-utils==0.4.0
ethereum-utils==0.3.2
gitpython==2.1.7
idna==2.5
Jinja2==2.9.6
json-rpc==1.10.3
Expand Down