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

Update poa-bridge to use v2 contracts #103

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DrPeterVanNostrand
Copy link
Contributor

Problem: poa-bridge is not compatible with v2 bridge-contracts

Solution: update deposit-relay, withdraw-relay, and message-to-mainnet to be compatible with v2 contracts, remove withdraw-confirm, create deposit-confirm.

Closes #5

Solution: update deposit-relay, withdraw-relay, and
message-to-mainnet to be compatible with v2 contracts, remove
withdraw-confirm, create deposit-confirm.

Closes #5
@DrPeterVanNostrand
Copy link
Contributor Author

DrPeterVanNostrand commented Jun 11, 2018

I've run into a few problems that maybe someone can help me with:

  1. How should bridge/build.rs be updated to accommodate for the v2 contracts?

Right now, it compiles the bridge.sol contract (which is not v2 compatible) into the folder: compiled-contracts/.

Command::new("solc")
	.arg("--abi")
	.arg("--bin")
	.arg("--optimize")
	.arg("--output-dir").arg("../compiled_contracts")
	.arg("--overwrite")
	.arg("../contracts/bridge.sol")
	.status()

I have been using the poa-bridge-contracts repo for compiling and deploying v2 contracts, then I just copy the HomeBridge, ForeignBridge and ERC20abi files into the folder:poa-bridge/compiled-contracts/.

I think that we should remove the contract compilation step from the bridge's build script, and add a paragraph to README.md with instructions on how to clone and compile the new contracts.

  1. Should the deploy module and feature be removed, or should it be updated to v2 as well? This PR did not update any of the deploy stuff as I was told it was a deprecated method of deployment and it was outside of the scope of issue (Epic) Reverse bridge logic in order to do all expensive confirmations on HomeContract side #5. Right now, deploy.rs is still only compatible with v1 contracts.

@yrashk
Copy link
Contributor

yrashk commented Jun 11, 2018

Deploy feature it's absolutely going to be removed, there's no point in it with this change. Basically, we just need to make the integration tests use the new deployment method to provision.

@rstormsf
Copy link
Contributor

It's the right direction to move forward

@DrPeterVanNostrand
Copy link
Contributor Author

DrPeterVanNostrand commented Jun 11, 2018

Should I remove it with this PR then? If you are using the v2 contracts and this PR, you will get build errors due to deploy.rs using the v1 contracts and the now removed withdraw_confirm module.

@yrashk
Copy link
Contributor

yrashk commented Jun 11, 2018 via email

@DrPeterVanNostrand
Copy link
Contributor Author

@yrashk Ok, I'll add those.

@yrashk
Copy link
Contributor

yrashk commented Jun 11, 2018 via email

@DrPeterVanNostrand
Copy link
Contributor Author

Ok, thanks. I'll let you know if I hit issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants