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

Live test fixes #260

Merged
merged 22 commits into from
Jul 26, 2019
Merged

Live test fixes #260

merged 22 commits into from
Jul 26, 2019

Conversation

saschagoebel
Copy link
Contributor

Many small changes and fixes related to running the app in "production"

It's probably easier to just look at the single commits to see what has changed and why.

While sorting the config parameters it showed, that the naming could be improved a bit.
Suggestions:

  • home_chain_event_fetch_start_block_number -> home_bridge_event_fetch_start_block_number
    • This value is directly related to home_bridge_contract_address
  • foreign_chain_event_fetch_start_block_number-> foreign_bridge_event_fetch_start_block_number
    • This value is directly related to foreign_bridge_contract_address
  • token_contract_address -> foreign_chain_token_contract_address
    • Just to make clear where this contract lives

See the current CONFIG_ENTRY_VALIDATORS list. The suggestions are pretty obvious there.
@schmir-at-bb Shall we change these?

@saschagoebel saschagoebel requested review from weilbith and a user July 25, 2019 07:29
Copy link
Contributor

@weilbith weilbith 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. I would like to keep this open until the test bridge deployment is working. Because this is already a collection of multiple small and not related changes.

@ghost
Copy link

ghost commented Jul 25, 2019

  • home_chain_event_fetch_start_block_number -> home_bridge_event_fetch_start_block_number

    • This value is directly related to home_bridge_contract_address
  • foreign_chain_event_fetch_start_block_number-> foreign_bridge_event_fetch_start_block_number

    • This value is directly related to foreign_bridge_contract_address

The start_block_number refers to a value on the chain. I would rather not rename them.

  • token_contract_address -> foreign_chain_token_contract_address

    • Just to make clear where this contract lives

this is fine for me.

raise ValueError(f"{url} is not a valid RPC url")
return url


def validate_non_negative_integer(number: Any) -> float:
def validate_non_negative_integer(number: Any) -> int:
if str(number) != str(int(number)):
Copy link

Choose a reason for hiding this comment

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

This check looks bogus. What should it do? Same below.
I do have the feeling that I could help with the function, but I'm not sure what kind of arguments it should be able to handle? string, int and float?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

String and int. The float version should handle string, float and int.

This is mostly to satisfy the tests. I want to input a "number" and it should be positive.

Copy link

Choose a reason for hiding this comment

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

Sorry, but I still don't get it, what this check does. This will filter out floats, anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It satisfies this test

The question is, does the test make sense, or should the function just use 1 and continue?

tools/bridge/requirements.txt Show resolved Hide resolved
@saschagoebel
Copy link
Contributor Author

The validation issue has not been resolved, but I'd like to merge this branch soon-ish, because it contains some necessary fixes to run the bridge.

The chainId problem is still open, but can hopefully be resolved by #262

All in all the changes do not break anything and can be merged to build upon. we can create a separate issue for the Validation stuff maybe?

@ghost ghost added this to the 25.07.2019 milestone Jul 25, 2019
@ghost
Copy link

ghost commented Jul 25, 2019

The validation issue has not been resolved, but I'd like to merge this branch soon-ish, because it contains some necessary fixes to run the bridge.

The chainId problem is still open, but can hopefully be resolved by #262

All in all the changes do not break anything and can be merged to build upon. we can create a separate issue for the Validation stuff maybe?

merge #263 and feel free to merge this PR. I will take care of the validation stuff.

ethpm has been integrated into web3. That means we can get rid of the
workaround for missing dependencies.

Before upgrading to this version, the standalone ethpm package has to
be manually uninstalled.

The following PR has been merged into 5.0.0b4, which is the primary
reason for the upgrade:

  ethereum/web3.py#1378
@saschagoebel saschagoebel merged commit fb1bfa3 into master Jul 26, 2019
@saschagoebel saschagoebel deleted the live-test-fixes branch July 26, 2019 07:09
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.

3 participants