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

protocol contract deployment verification via CI #1627

Merged
merged 10 commits into from
Feb 3, 2023
Merged

protocol contract deployment verification via CI #1627

merged 10 commits into from
Feb 3, 2023

Conversation

Ashar2shahid
Copy link
Contributor

@Ashar2shahid Ashar2shahid commented Jan 30, 2023

Closes #1594

This add CI Verification to the protocol contracts using the verify-local script. I opted to return the list of all failed verifications and then throw the error instead of throwing an error at the first failed verification.

  • I created a separate workflow file for deployment-verification due to the path filter only being applicable on a per workflow basis, this means that if we want to run the CI only if the airnode-protocol folder changes then we need a separate workflow.
  • we can use the paths-filer action if we want to do the deployment-verification and check if the airnode-protocol folder has changed in the Continuous Build workflow but its a third-party.
  • we can opt out of checking if the airnode-protocol folder has changed and run the CI on every workflow-run by incorporating it as an extra step of the Continuous Build workflow

@Ashar2shahid
Copy link
Contributor Author

Ashar2shahid commented Jan 30, 2023

As seen here: https://github.com/api3dao/airnode/actions/runs/4041541899/jobs/6948239143 the CI is failing because of :

  • some chains not existing (roptsen,rinkeby,kovan)
  • going down frequently (gnosis-testnet)
  • the RPCs being faulty (polygon)

Copy link
Member

@bbenligiray bbenligiray left a comment

Choose a reason for hiding this comment

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

I took a brief look, I can review again after #1562 is merged

Just remove the ropsten, rinkeby, kovan deployments. It should have been done as a part of #1280 While at it, remove gnosis-testnet and metis-testnet too. They can be added back in a separate PR to close #1628.

@@ -19,10 +19,10 @@
"deploy:deterministic": "DETERMINISTIC=true hardhat deploy --network $NETWORK --tags deploy && yarn run deploy:generate-references",
"deploy:undeterministic": "hardhat deploy --network $NETWORK --tags deploy && yarn run deploy:generate-references",
"deploy:verify": "hardhat deploy --network $NETWORK --tags verify",
"deploy:verify-local": "hardhat run scripts/verify-local.ts --network $NETWORK",
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to move this under test:, but the single network local verification came in handy while doing the deployments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool I'll try to accommodate both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use test:verify-local:network for single network verification

@bbenligiray
Copy link
Member

Can you merge #1562 and merge master to 1594 so that we know that the CI also works for the new chains?

@Ashar2shahid
Copy link
Contributor Author

Checks are passing, good to merge

Copy link
Member

@bbenligiray bbenligiray left a comment

Choose a reason for hiding this comment

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

I think the implementation in verify-local.ts could be better compartmentalized to be more reusable. The current thing addresses the issue though so maybe someone will come back to this later.

@amarthadan amarthadan merged commit 99c5927 into master Feb 3, 2023
@amarthadan amarthadan deleted the 1594 branch February 3, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add protocol contract deployment verification to CI
3 participants