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

Görli contracts deployment #387

Merged
merged 16 commits into from
Aug 9, 2022
Merged

Görli contracts deployment #387

merged 16 commits into from
Aug 9, 2022

Conversation

nkuba
Copy link
Member

@nkuba nkuba commented Jul 20, 2022

Görli became a recommended test network after Ropsten's deprecation notice (link).

In this PR we update deployment scripts and configuration to support Görli.

The contracts were already deployed to Görli:
@keep-network/[email protected]

nkuba added 8 commits July 20, 2022 13:16
For the Relay we renamed the artifact name to BitcoinRelay as the
previous one was ambiguous and could be confused with other contracts we
use in dependency projects.
While deploying to Goerli with Alchemy we experienced some problems with
transactions being rejected as the state of the chain wasn't reflecting
the previously executed transaction. We added confirmation wait to let
the Alchemy nodes propagate the transaction across their nodes.
We're not yet ready with the relay setup on goerli so we want to have a
stub for now.
We expect some contraccts from other modules to be pre-deployed. Here we
specify patsh where to find the artifacts.
We have USE_EXTERNAL_DEPLOY flag to explictly determine where we want to
use the external deployment scripts.
@nkuba nkuba changed the title Goerli deploy Görli contracts deployment Aug 2, 2022
nkuba added 2 commits August 2, 2022 14:45
We add wait for running transactions on other networks than the hardhat
    local network, e.g. development geth node or goreli.
@nkuba nkuba marked this pull request as ready for review August 2, 2022 13:22
Copy link
Contributor

@michalinacienciala michalinacienciala left a comment

Choose a reason for hiding this comment

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

Generally, the changes look good to me. But there is some problem with the deployment on goerli - I'm getting:

initializing Bridge as ECDSA wallet owner
An unexpected error occurred:
Error: ERROR processing /home/runner/work/tbtc-v2/tbtc-v2/solidity/deploy/16_initialize_wallet_owner.ts:
TypeError: Cannot read property 'length' of undefined

This doesn't seem to be directly caused by the changes in this PR, it looks more like a problem with keep-network/keep-core#3061. @nkuba, do you think that merging of keep-network/keep-core#3096 may help?

@nkuba
Copy link
Member Author

nkuba commented Aug 3, 2022

Generally, the changes look good to me. But there is some problem with the deployment on goerli - I'm getting:

initializing Bridge as ECDSA wallet owner
An unexpected error occurred:
Error: ERROR processing /home/runner/work/tbtc-v2/tbtc-v2/solidity/deploy/16_initialize_wallet_owner.ts:
TypeError: Cannot read property 'length' of undefined

This doesn't seem to be directly caused by the changes in this PR, it looks more like a problem with keep-network/keep-core#3061. @nkuba, do you think that merging of keep-network/keep-core#3096 may help?

Interesting... We need to fix it in a separate PR to keep-core.

Actually bfd629c shuould fix it.

@nkuba nkuba self-assigned this Aug 3, 2022
@michalinacienciala
Copy link
Contributor

michalinacienciala commented Aug 4, 2022

Actually bfd629c should fix it.

Yep, I no longer see that error. There's a different error now though, see my comment keep-network/coverage-pools#215 (comment). Usually such errors were caused by some problems with the config - when deployer account from dependencies did not match the one used currently for deploy. But it doesn't seem like it this time... Any ideas?

[EDIT]: OK. I chatted with Kuba offline and he concluded that the error is related to the fact that for given ecdsa package that we use as dependency, we can call the initializeWalletOwner function only once (and it has already been called when @keep-network/[email protected] package was being prepared). This is because during deployment of tbtc-v2 contracts we're connecting Bridge with ecdsa's WalletRegistry and this connection can be made only once.
So in order to fully test the deployment of tbtc-v2 contracts on goerli we now need to have a new ecdsa package for goerli. We'll be able to publish it once keep-network/keep-core#3081 gets merged.

Copy link
Contributor

@michalinacienciala michalinacienciala left a comment

Choose a reason for hiding this comment

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

Deployment on goerli verified in job https://github.com/keep-network/tbtc-v2/runs/7744385930?check_suite_focus=true (workflow uses code combined from #387 and #382). Works as expected

@michalinacienciala michalinacienciala merged commit 445325d into main Aug 9, 2022
@michalinacienciala michalinacienciala deleted the goerli-deploy branch August 9, 2022 11:07
@pdyraga pdyraga added the ⛓️ solidity Solidity contracts label Jan 31, 2023
@pdyraga pdyraga added this to the solidity/v1.0.0 milestone Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⛓️ solidity Solidity contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants