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

Added section to Contributing.md detailing our desired pre-release code auditing process #1848

Merged
merged 13 commits into from
Jun 27, 2022
Merged
49 changes: 46 additions & 3 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,51 @@ For v6.x, and v4.x, most PRs to them should go to main and get a "backport" labe

### How to build proto files. (rm -rf vendor/ && make build-reproducible once docker is installed)

You can do rm -rf vendor and make build-reproducible to redownload all dependencies - this should pull the latest docker image of Osmosis. You should also make sure to do make proto-all to auto-generate your protobuf files. Makes ure you have docker installed.
You can do rm -rf vendor and make build-reproducible to redownload all dependencies - this should pull the latest docker image of Osmosis. You should also make sure to do make proto-all to auto-generate your protobuf files. Makes ure you have docker installed.

If you get something like `W0503 22:16:30.068560 158 services.go:38] No HttpRule found for method: Msg.CreateBalancerPool` feel free to ignore that.
If you get something like `W0503 22:16:30.068560 158 services.go:38] No HttpRule found for method: Msg.CreateBalancerPool` feel free to ignore that.

Make sure to also do make all to run all the linting tests before you commit and push, as well as `gofmt`-ing the file you've modified or added to make sure everything still abides by the standards.
You can also feel free to do `make format` if you're getting errors related to `gofmt`. Setting this up to be [automatic](https://www.jetbrains.com/help/go/reformat-and-rearrange-code.html#reformat-on-save) for yourself is recommended.

## Major Release

There are several steps that go into a major release

* Run the [existing binary creation tool](https://github.com/osmosis-labs/osmosis/blob/main/.github/workflows/release.yml). Running `make -f contrib/images/osmobuilder/Makefile release` on the root of the repo will replicate the CI that creates the release folder containing the binaries.

* Make a PR to main, with a cosmovisor config, generated in tandem with the binaries from tool.
* Should be its own PR, as it may get denied for Fork upgrades.

* Make a PR to main to update the import paths and go.mod for the new major release

* Should also make a commit into every open PR to main to do the same find/replace. (Unless this will cause conflicts)

* Do a PR if that commit has conflicts

* (Eventually) Make a PR that adds a version handler for the next upgrade
* [Add v10 upgrade boilerplate #1649](https://github.com/osmosis-labs/osmosis/pull/1649/files)

* Update chain JSON schema's recommended versions in `chain.schema.json` located in the root directory.

### Pre-release auditing process

For every module with notable changes, we assign someone who was not a primary author of those changes to review the entire module.

Deliverables of review are:

* PR's with in-line code comments for things they had to figure out (or questions)

* Tests / test comments needed to convince themselves of correctness

* Spec updates

* Small refactors that helped in understanding / making code conform to consistency stds / improve code signal-to-noise ratio welcome

* (As with all PRs, should not be a monolithic PR that gets PR'd, even though that may be the natural way its first formed)

At the moment, we're looking for a tool that lets us statically figure out every message that had something in its code path that changed. Until a tool is found, we must do this manually.

We test in testnet & e2e testnet behaviors about every message that has changed
Copy link
Member

Choose a reason for hiding this comment

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

test in e2e testnet about every message that has changed

I'm all for trying it but are we actually going to test every changed message in e2e before a major release? Is this an achievable commitment in practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @ValarDragon for thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I think this requires an extra step of testing but I'm not sure on what your thoughts are on the potential significance of overhead in patches where lots of messages are added (rather, how often these happen). If it's not too significant then it may be do-able.

Copy link
Member

@p0mvn p0mvn Jun 24, 2022

Choose a reason for hiding this comment

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

Please don't feel the need to block the PR on this comment if that's what's happening. I'm just reading between the lines.

I think that the commitment to manually test every changed message on testnet is a must. My concern is only with respect to e2e. Please consider not mentioning it here.

At the moment, we don't have the capacity to test every changed message on the e2e testnet. So the commitment might not be realistic. We should be slowly moving towards that over time so that we can minimize manual testnet testing.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine to list for now - We need to make capacity for testing changed messages on testnet & work towards mitigating # of changes per release / making smaller releases more reasonable imo


We communicate with various integrators if they'd like release-blocking QA testing for major releases
* Chainapsis has communicated wanting a series of osmosis-frontend functionalities to be checked for correctness on a testnet as a release blocking item