search

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

Problem: bridge panics if there's an error retrieving gas price #93

Closed yrashk closed 6 years ago

yrashk commented 6 years ago

Solution: instead, log an error and use previous price

DrPeterVanNostrand commented 6 years ago

@akolotov To test, I just ran cargo test basic and cd integration-tests && cargo test -- --test-threads=1 --nocapture. However, not all integration tests were passing.

When testing, I logged out the retrieved gas prices and verified them manually, by going to the Oracle url in my browser. This is definitely not the ideal way to test, but I found that the test suite was pretty confusing (in code layout, what the tests were printing out, sometimes a parity node would be left running in the background after a test failed).

With the changes in this PR, the two error cases for the dynamic gas-price (http request error, unrecognized JSON schema) will just default to the last retrieved price. So i'm not sure if adding any new tests would be beneficial. Do you guys @akolotov @yrashk have any suggestions for better ways to do testing around something like this?

akolotov commented 6 years ago

@DrPeterVanNostrand is it enough to run just integration tests to cover all possible error cases addressed in the changes? Just would like to get sure that we confirm that changes are safe before running them on bridge-testnet since deployment and testing there require at least one person-day of work. Are you able to run the bridge instance on real contracts (https://github.com/poanetwork/poa-bridge-contracts/releases/tag/1.0) and try to disconnect (e.g. through hosts) the oracle, provide incorrect output from oracle etc.?

yrashk commented 6 years ago

@akolotov I ran a number of manual tests on getting wrong responses from an oracle. However, I'm working on decomposing the gas price retriever to be able to simulate these scenarios in automated tests.

yrashk commented 6 years ago

I've added automatic tests and switched to gas price specific timeout

DrPeterVanNostrand commented 6 years ago

@akolotov I don't have anything else to add