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

Proxy fixes #1530

Merged
merged 4 commits into from Jun 8, 2018
Merged

Proxy fixes #1530

merged 4 commits into from Jun 8, 2018

Conversation

palango
Copy link
Contributor

@palango palango commented Jun 8, 2018

Use the raiden-contracts ContractManager for now and disable version checks.

Also fix some styling and add some type annotations.

@pcppcp
Copy link
Contributor

pcppcp commented Jun 8, 2018

this might cause some confusion, because two instances of ChannelManager will be used in the app

@palango
Copy link
Contributor Author

palango commented Jun 8, 2018

this might cause some confusion, because two instances of ChannelManager will be used in the app

Yup, that may be. Would you prefer to wait till we remove the old contracts?

@pcppcp
Copy link
Contributor

pcppcp commented Jun 8, 2018

I think we can merge it if we want to use contracts in parallel... we'll eventually switch to wrapped manager when the old contracts are removed.

@@ -51,7 +51,7 @@
CONTRACT_NETTING_CHANNEL = 'NettingChannelContract'
CONTRACT_REGISTRY = 'Registry'

CONTRACT_TOKEN_NETWORK_REGISTRY = 'TokenNetworkRegistry'
CONTRACT_TOKEN_NETWORK_REGISTRY = 'TokenNetworksRegistry'
Copy link
Contributor

Choose a reason for hiding this comment

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

The name should be fixed in the smart contracts, not here, as suggested by Lefteris. Issue: raiden-network/raiden-contracts#107

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, please switch to singular

@@ -51,7 +51,7 @@
CONTRACT_NETTING_CHANNEL = 'NettingChannelContract'
CONTRACT_REGISTRY = 'Registry'

CONTRACT_TOKEN_NETWORK_REGISTRY = 'TokenNetworkRegistry'
CONTRACT_TOKEN_NETWORK_REGISTRY = 'TokenNetworksRegistry'
Copy link
Contributor

Choose a reason for hiding this comment

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

yep, please switch to singular

@@ -36,8 +36,8 @@ def __init__(
self,
privatekey_bin: bytes,
jsonrpc_client: JSONRPCClient,
poll_timeout: int = DEFAULT_POLL_TIMEOUT):

poll_timeout: int = DEFAULT_POLL_TIMEOUT
Copy link
Contributor

Choose a reason for hiding this comment

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

hehe, you bothered fixing style but forgot the trailing comma in the last argument

CONTRACT_SECRET_REGISTRY
)

# TODO: add this back
Copy link
Contributor

Choose a reason for hiding this comment

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

If you leave a TODO like that please make an issue and assign yourself (or someone else). Same goes for the other instances below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think that's worth an issue right now, added a task here: #1383

Copy link
Contributor

Choose a reason for hiding this comment

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

@palango Whatever floats your boat. It's just to keep track of them. If it's part of an issue adding a task is fine. Thank you!

):
web3 = deploy_client.web3

contract_interface = CONTRACT_MANAGER.abi['SecretRegistry']
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use the contract names directly - use the imported names from the raiden-contracts package (ideally) or blockchain/abi.py

@coveralls
Copy link

coveralls commented Jun 8, 2018

Coverage Status

Coverage decreased (-6.08%) to 64.895% when pulling 221916f on palango:proxy-fixes into 782b4d4 on raiden-network:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.004%) to 69.968% when pulling 571b934 on palango:proxy-fixes into 782b4d4 on raiden-network:master.

palango and others added 2 commits June 8, 2018 16:29
We have to increase the block gas limit for that.
@@ -7,7 +7,7 @@
RPC_CACHE_TTL = 600
CACHE_TTL = 60
ESTIMATED_BLOCK_TIME = 7
GAS_LIMIT = 3141592 # Morden's gasLimit.
GAS_LIMIT = 10 * 10**6
Copy link
Contributor

Choose a reason for hiding this comment

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

It's all fine @palango, so I will merge but why did you increase the default gas limit? The 3.14 mill was really old a value, that's true -- ropsten now has 8 mill. But why 10? Any specific reason? I am curious.

Copy link
Contributor

@LefterisJP LefterisJP Jun 8, 2018

Choose a reason for hiding this comment

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

@palango I just saw the commit that explains that you needed bigger gas limit for the new deployment so this is why you made the change. How much gas was needed exactly?

Copy link
Contributor

Choose a reason for hiding this comment

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

4.8 mil last I know of - for the TokenNetworkRegistry.

@LefterisJP LefterisJP merged commit 03fd1fc into raiden-network:master Jun 8, 2018
@palango palango deleted the proxy-fixes branch May 28, 2019 13:22
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.

None yet

5 participants