Feat: Re-enabling gravity bridge conditionally

[thomas-nguy]

We probably needs to add more test in case gravity is not enable

PR Checklist:

• Have you read the CONTRIBUTING.md?
• Does your PR follow the C4 patch requirements?
• Have you rebased your work on top of the latest master?
• Have you checked your code compiles? (make)
• Have you included tests for any non-trivial functionality?
• Have you checked your code passes the unit tests? (make test)
• Have you checked your code formatting is correct? (go fmt)
• Have you checked your basic code style is fine? (golangci-lint run)
• If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
• If your changes affect the client infrastructure, have you run the integration test?
• If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
• If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
• If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

[codecov]

Codecov Report

Merging #210 (59dab41) into main (3ea70c5) will increase coverage by 3.77%.
The diff coverage is 38.97%.

@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
+ Coverage   21.51%   25.29%   +3.77%     
==========================================
  Files          27       34       +7     
  Lines        1729     2554     +825     
==========================================
+ Hits          372      646     +274     
- Misses       1324     1858     +534     
- Partials       33       50      +17     

Impacted Files	Coverage Δ
app/prefix.go	0.00% <0.00%> (ø)
app/test_helpers.go	0.00% <0.00%> (ø)
x/cronos/keeper/gravity_hooks.go	0.00% <ø> (ø)
x/cronos/keeper/grpc_query.go	0.00% <ø> (ø)
x/cronos/keeper/ibc.go	83.20% <ø> (+5.01%)	⬆️
x/cronos/keeper/ibc_hooks.go	50.00% <ø> (-8.83%)	⬇️
x/cronos/keeper/keeper.go	64.89% <ø> (-29.45%)	⬇️
x/cronos/keeper/msg_server.go	4.76% <ø> (-1.69%)	⬇️
x/cronos/module.go	57.62% <ø> (-4.20%)	⬇️
x/cronos/proposal_handler.go	6.25% <ø> (ø)
... and 28 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 087d3cd...59dab41. Read the comment docs.

[thomas-nguy]

@tomtau should the ADR be updated as well?

[tomtau]

@tomtau should the ADR be updated as well?

If there's a need for it, yes: the record doesn't say anything about conditional compilation: https://github.com/crypto-org-chain/cronos/blob/main/docs/architecture/adr-001.md#decision

The original draft had the conditional compilation, but from the discussion, it turned out that there was no need for it:

• the current "mainnet beta" functionality can be forked into a "release/v0.6.x" branch and potential non-breaking changes from "main" can be cherry-picked in PRs onto it (e.g. switching to the next 0.7 Ethermint upstream release)
• the latest/testnet development can just continue on main

[thomas-nguy]

Per discussion, I thought that we wanted the conditional compilation because we still don't know when gravity will be mature enough to be used in production and there are chances that breaking changes will need to be ported to mainnet before that

If we want to avoid maintaining multiple fork or doing cherry picking, this would be the easiest solution?

[tomtau]

Per discussion, I thought that we wanted the conditional compilation because we still don't know when gravity will be mature enough to be used in production and there are chances that breaking changes will need to be ported to mainnet before that

If we want to avoid maintaining multiple fork or doing cherry picking, this would be the easiest solution?

There's a need to maintain a release branch for 0.6.x once "main" runs on the latest Ethermint and has /x/feemarket added. So a release branch is likely expected or unavoidable irrespective of GB.
If we anticipate GB isn't immediately added in a network upgrade altogether with other breaking changes on "main" and we need to keep up the latest development with it (e.g. for testnet binaries)... then yes, conditional feature-guarded compilation makes sense and this clarification should be updated to ADR-001.

[tomtau]

Suggested change

	* 13-10-2021: Conditional compilation
	* 16-11-2021: Conditional compilation

[tomtau]

should those (module version + upgrade handler) be conditionally added/changed as well?

[thomas-nguy]

I think we can avoid this for now.

We could keep the same consensus version for both binary (with and without gravity) and increase it when we decide to release gravity to mainnet (at that time, we can also remove the conditional compilation)

[tomtau]

ok, then it's good to move or rephrase this sentence

[thomas-nguy]

sure, changed

[yihuang]

But I guess the integration tests running in CI only target one of the modes, the other mode will not be tested.
For example if we pick the gravity-bridge mode to be the default one, the non-gravity-bridge mode won't be tested in CI, and could be silently broken.
Unless we run the tests against both modes, will that be overkill?

[thomas-nguy]

Yeah as I mentioned in the PR, we need to open another issue to improve the integration tests.
I guess the easiest way is to add another build + integration tests run in the CI for non gravity bridge binary?

What do you think?

[yihuang]

Yeah as I mentioned in the PR, we need to open another issue to improve the integration tests. I guess the easiest way is to add another build + integration tests run in the CI for non gravity bridge binary?

What do you think?

Do you think the two modes are temporary or permanent? my concern is it's not easy to keep both modes working correctly in the long run, and there could be more *_nogravity.go duplicated modules in the future when we add something only for one of the modes.

[thomas-nguy]

Do you think the two modes are temporary or permanent? my concern is it's not easy to keep both modes working correctly in the long run.

I guess once we decide to launch main net with gravity bridge, we don't need the conditional build anymore and can just stick with one version

[yihuang]

Do you think the two modes are temporary or permanent? my concern is it's not easy to keep both modes working correctly in the long run.

I guess once we decide to launch main net with gravity bridge, we don't need the conditional build anymore and can just stick with one version

Then I guess using branches should be good enough?

• Before we decide to use the gravity bridge, we develop that in a feature branch.
• After we decide to use the gravity bridge, we create a release branch for the non-gravity-bridge version, and merge the gravity bridge version to main branch

[thomas-nguy]

Yeah this is possible but the conditional build is just to avoid us to jungle with two branches and cherry-pick back and forth for them to stay sync for a certain amount of time.

Also less risk to mess up during the upgrade?

But if we decide to go that way, I can remove the conditional build. What do you think @tomtau @calvinaco

[tomtau]

if this repo follows TBD, then long-lived feature branches aren't desirable.
if there is a concern for not always building with the feature on, it can be a temporary runtime flag: https://trunkbaseddevelopment.com/feature-flags/ -- and selected E2E or integration tests can be re-executed with that flag on

[yihuang]

if this repo follows TBD, then long-lived feature branches aren't desirable.

It's temporary, it'll be merged to the main branch when we decide to use gravity-bridge in the next release.
and after that the non-gravity-bridge branch is only for bug fixes before the mainnet upgraded to gravity-bridge.

[tomtau]

if temporary feature branch == a few days, then it's all right as TBD: https://trunkbaseddevelopment.com/youre-doing-it-wrong/#duration-of-short-lived-feature-branches

Otherwise, the contribution guidelines may need changes: https://github.com/crypto-org-chain/cronos/blob/main/CONTRIBUTING.md (which branch should one open a PR against if there are two parallel developments? who/when/what back ports changes from one to the other? how are releases made ? ...)

[yihuang]

if temporary feature branch == a few days, then it's all right as TBD: https://trunkbaseddevelopment.com/youre-doing-it-wrong/#duration-of-short-lived-feature-branches

Otherwise, the contribution guidelines may need changes: https://github.com/crypto-org-chain/cronos/blob/main/CONTRIBUTING.md (which branch should one open a PR against if there are two parallel developments? who/when/what back ports changes from one to the other? how are releases made ? ...)

I guess before we decide to use gravity bridge, there won't be heavy development on it anyway.
I think the blocker of that decision is to evaluate different gravity bridge forks, right? it seems good to do the evaluation on a feature branch rather than the main branch.

[yihuang]

LGTM