omni / poa-bridge

POA <-> Ethereum bridge for self transfers of POA native token to POA20 (ERC20 representation). Not supported. Use TokenBridge instead
https://github.com/poanetwork/token-bridge
GNU General Public License v3.0
79 stars 38 forks source link

Update poa-bridge to use v2 contracts #103

Open DrPeterVanNostrand opened 6 years ago

DrPeterVanNostrand commented 6 years ago

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

DrPeterVanNostrand commented 6 years ago

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 #5. Right now, deploy.rs is still only compatible with v1 contracts.
yrashk commented 6 years ago

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 commented 6 years ago

It's the right direction to move forward

DrPeterVanNostrand commented 6 years ago

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 commented 6 years ago

Yes, I think this PR should drop deploy altogether and invoke deployment scripts from v2 from within integration tests.

Also, build.rs can be replaced with just putting submoduled or directly committed ABIs for v2.

On Mon, Jun 11, 2018, 11:46 AM DrPeterVanNostrand notifications@github.com wrote:

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

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/poanetwork/poa-bridge/pull/103#issuecomment-396345736, or mute the thread https://github.com/notifications/unsubscribe-auth/AAABxHC_3XIooBDI92A4zyW_6F65yDvvks5t7rrpgaJpZM4UjBXC .

DrPeterVanNostrand commented 6 years ago

@yrashk Ok, I'll add those.

yrashk commented 6 years ago

I'm also happy to assist if the tests need to be adjusted.

On Mon, Jun 11, 2018, 11:54 AM DrPeterVanNostrand notifications@github.com wrote:

@yrashk https://github.com/yrashk Ok, I'll add those.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/poanetwork/poa-bridge/pull/103#issuecomment-396348318, or mute the thread https://github.com/notifications/unsubscribe-auth/AAABxIfojXy2KifiYQHqCv_q6iM7Tj_Vks5t7rzkgaJpZM4UjBXC .

DrPeterVanNostrand commented 6 years ago

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