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

Scripts for verifying bytecodes #5111

Merged
merged 81 commits into from Oct 8, 2020
Merged

Scripts for verifying bytecodes #5111

merged 81 commits into from Oct 8, 2020

Conversation

m-chrzan
Copy link
Contributor

@m-chrzan m-chrzan commented Sep 17, 2020

Description

Adds two bash wrappers around verify-bytecodes:

  • verify-deployed checks that the current Celo contracts correspond to Solidity sources on a given branch.
  • verify-release checks that after executing a given proposal, the onchain contracts will correspond to Solidity sources on a given branch.

Other changes

Added a -f flag to several of our bash scripts, and modified truffle-config to take a --forno parameter that allows Truffle scripts to run with a Forno provider on testnets/the mainnet for which a Forno instance exists.

Added comments, cleaned up scripts.

Tested

verify-deployed

celotooljs port-forward -e baklava
yarn verify-deployed -n baklava -b baklava -f

verify-release

git checkout rc1
yarn devchain generate-tar devchain.tar.gz
git checkout -
yarn devchain run-tar devchain.tar.gz
yarn make-release -a rc1 -b m-chrzan/bytecode-scripts -n development -p proposal.json -i example-initialize-data.json -r report.json
yarn verify-release -b m-chrzan/bytecode-scripts -n development -p proposal.json

Related issues

Backwards compatibility

+1

@aaronmgdr aaronmgdr removed their request for review September 22, 2020 20:03
@nategraf nategraf removed their request for review September 23, 2020 00:12
@yorhodes yorhodes changed the base branch from m-chrzan/verify-release to master September 25, 2020 22:21
@yorhodes yorhodes changed the base branch from master to m-chrzan/verify-release September 25, 2020 22:22
@yorhodes yorhodes changed the base branch from m-chrzan/verify-release to master October 5, 2020 17:17
Copy link
Contributor

@nambrot nambrot left a comment

Choose a reason for hiding this comment

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

Amazing!

# We fetch via HTTPS instead of SSH to avoid the interactive "do you
# trust the RSA key fingerprint" prompt.
git remote add origin-https https://github.com/celo-org/celo-monorepo.git
git fetch origin-https rc1
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, this is incorrect, as master needs to compare against release 1.

I believe there are only 3 options:

  1. Adjust the migrations to proxy linked libraries and then checkout the release-1 branch here
  2. Create a special case for release 2, where creating the bytecode involves
    2.1 Deploying RC1
    2.2 Creating the proposal for release 1
    2.3 Get through the proposal for release 1
    2.4 Continue
  3. Not add this to CI until 1. is done

Copy link
Contributor

@yorhodes yorhodes Oct 8, 2020

Choose a reason for hiding this comment

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

went with option 3

command: |
./scripts/ci_check_if_test_should_run_v2.sh @celo/protocol
- run:
name: Verify bytecodes between RC1 and current branch
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: Verify bytecodes between RC1 and current branch
name: Verify bytecodes between most recent release and current branch

packages/docs/community/release-process/smart-contracts.md Outdated Show resolved Hide resolved
# deploy new contracts and generate governance proposal for upgrade
yarn truffle exec --network development ./scripts/truffle/make-release.js --build_directory build/ --report report.json --proposal proposal.json --initialize_data example-initialize-data.json'
NETWORK=${"baklava"|"alfajores"|"mainnet"}
RELEASE="celo-core-contracts-v${N}.${NETWORK}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RELEASE="celo-core-contracts-v${N}.${NETWORK}"
RELEASE="celo-core-contracts-v${N-1}.${NETWORK}"

I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to generalize this and add the specific instruction in backward compatibility check but this is probably the primary use case

RELEASE="celo-core-contracts-v${N}.${NETWORK}"
# A -f boolean flag can be provided to use a forno full node to connect to the provided network
# A -r boolean flag should be provided if this is the first release (before linked libraries were proxied)
yarn verify-deployed -n $NETWORK -b $RELEASE -r -f
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's in scope for this PR, but for these critical scripts, I'm not sure I'm a fan of these shortened flags, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

will address in followup PR

packages/protocol/scripts/bash/check-versions.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@yorhodes yorhodes left a comment

Choose a reason for hiding this comment

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

LGTM

@yorhodes yorhodes added the automerge Have PR merge automatically when checks pass label Oct 8, 2020
@mergify mergify bot merged commit 41d3d03 into master Oct 8, 2020
@mergify mergify bot deleted the m-chrzan/bytecode-scripts branch October 8, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
5 participants