search

keep-network / local-setup

Allow to easily set up tBTC and Keep locally for development and test purposes.
MIT License
8 stars 16 forks source link

Support EIP-155 in E2E test #122

Closed nkuba closed 2 years ago

nkuba commented 2 years ago

We update dependencies to the latest versions to support EIP-155 replay-protected transactions.

PrivateKeyWalletSubprovider expects chainId to be provided as a constructor argument. If the value is not provided the provider assumes it deals with mainnet (chainId = 1).

After EIP-155 the chainId is included when calculating a signature, and transactions that are sent to the Ethereum endpoint with non-matching chainId are rejected with invalid sender.

The previous solution worked fine with Ethereum endpoint that had replay protection disabled, but once the protection is enabled we need to include the correct chainId in the transaction signature.

nkuba commented 2 years ago

I invoked a test run for this branch to see if the fix works: https://github.com/keep-network/local-setup/runs/3789133012

michalinacienciala commented 2 years ago

I invoked a test run for this branch to see if the fix works: https://github.com/keep-network/local-setup/runs/3789133012

I see that the job has failed during installation of keep-ecdsa, which is strange, because your changes didn’t touch that area (and E2E tests on main are failing at later stage) :thinking_face:

michalinacienciala commented 2 years ago

I invoked a test run for this branch to see if the fix works: https://github.com/keep-network/local-setup/runs/3789133012

I see that the job has failed during installation of keep-ecdsa, which is strange, because your changes didn’t touch that area (and E2E tests on main are failing at later stage) :thinking_face:

@nkuba, it looks that the failure was not related to your changes - it was a result of migration random-beacon code from solidity/ to solidity-v1/ in keep-core. I crated a PR that should fix it, I'm testing it right now. Once I confirm, we can merge the changes to main, update your branch and run the tests here again.

nkuba commented 2 years ago

I invoked a test run for this branch to see if the fix works: https://github.com/keep-network/local-setup/runs/3789133012

I see that the job has failed during installation of keep-ecdsa, which is strange, because your changes didn’t touch that area (and E2E tests on main are failing at later stage) :thinking_face:

@nkuba, it looks that the failure was not related to your changes - it was a result of migration random-beacon code from solidity/ to solidity-v1/ in keep-core. I crated a PR that should fix it, I'm testing it right now. Once I confirm, we can merge the changes to main, update your branch and run the tests here again.

Thanks for that @michalinacienciala ! I updated this branch and invoked the job again https://github.com/keep-network/local-setup/actions/runs/1319975000 🤞🏻

michalinacienciala commented 2 years ago

Thanks for that @michalinacienciala ! I updated this branch and invoked the job again https://github.com/keep-network/local-setup/actions/runs/1319975000 🤞🏻

@nkuba, I see that the workflow is failing with following error:

Starting deposit number [1]...

Test errored out with error:  ReferenceError: web3 is not defined
    at createDeposit (file:///home/runner/work/local-setup/local-setup/e2e/e2e-test.js:263:5)

Any idea if it is smth related to the EIP-155 change?

nkuba commented 2 years ago

Next try: https://github.com/keep-network/local-setup/actions/runs/1434707961

michalinacienciala commented 2 years ago

Next try: https://github.com/keep-network/local-setup/actions/runs/1434707961

Unfortunately it's still failing with this web3 is not defined issue...

nkuba commented 2 years ago

Next try: https://github.com/keep-network/local-setup/actions/runs/1434707961

Unfortunately it's still failing with this web3 is not defined issue...

Of course, it does fail. I forgot to commit the change 😅 I'm sorry, one more try! 🤞🏻 https://github.com/keep-network/local-setup/actions/runs/1439292511

michalinacienciala commented 2 years ago

Of course, it does fail. I forgot to commit the change 😅 I'm sorry, one more try! 🤞🏻 https://github.com/keep-network/local-setup/actions/runs/1439292511

This time we've got this:

Starting deposit number [1]...

Test errored out with error:  TypeError: Cannot read property 'toBN' of undefined
    at createDeposit (file:///home/runner/work/local-setup/local-setup/e2e/e2e-test.js:265:16)
michalinacienciala commented 2 years ago

@nkuba, what's the status of this PR? Any chance for getting the tests fixed?