POC: feature gate for zone conceirge module

[SebastianElvis]

This PR provides a PoC for the feature gate of zone concierge. In particular,

• It introduces a new module parameter enable_integration. If set true then integration is enabled.
• Disabling the integration means that 1) post handler will do nothing, 2) IBC channel creation with zone concierge is disabled, and 3) inclusion proofs won't be generated. Here 1 disables phase-1 timestamping integration, and 2/3 disables phase-2 timestamping integration.

Excluding the proto/swagger stuff, this is a change of 30 LoCs. Some modifications might not be necessary but adding them for being extra cautious.

This feature gate is already reflected in failed unit/timestamping e2e tests -- by default zone concierge no longer indexes IBC headers, or approves any IBC channel creation request.

[KonradStaniec]

so there are few specific things that worry me about this approach:

1. By putting new field in params we need to do the upgrade from TGE chain to now contain his modified version of params.
2. By putting the enable/disable flag to parameters we signal that integration can be enabled/disabled at will. So in theory it could be enabled in phase-2 if validators wanted it, even though this would be un-tested code path which we do not intend to support.
3. (This may be my hazy understading of timestamping integration, so correct me if I am wrong 😅 ) If we disable proofs generation, wouldn't be need to backfill those in upgrade eitherway ?

What your opinion on having EnableIntegration flag hardcoded in binary at app.go level ? Then:

• we do not need upgrade
• there is not way to enable it without releasing new version
• for ibc messages we can disable routing at app.go level which seems like safer option i.e instead of:

	ibcRouter := porttypes.NewRouter().
		AddRoute(ibctransfertypes.ModuleName, transferStack).
		AddRoute(zctypes.ModuleName, zoneConciergeStack).
		AddRoute(wasmtypes.ModuleName, wasmStack)

have:

	ibcRouter := porttypes.NewRouter().
		AddRoute(ibctransfertypes.ModuleName, transferStack).
		AddRoute(wasmtypes.ModuleName, wasmStack)


	if !EnableIntegration {
		ibcRouter = ibcRouter.AddRoute(zctypes.ModuleName, zoneConciergeStack)
	}

[SebastianElvis]

so there are few specific things that worry me about this approach:

1. By putting new field in params we need to do the upgrade from TGE chain to now contain his modified version of params.

This is actually fine -- the field uses a new identifier 2 so in theory upon upgrade the params bytes can still be marshaled, and this value will be by default false. I could verify this though.

2. By putting the enable/disable flag to parameters we signal that integration can be enabled/disabled at will. So in theory it could be enabled in phase-2 if validators wanted it, even though this would be un-tested code path which we do not intend to support.

This is a valid concern. That's why I mentioned we could move it as a genesis param rather than module param. This way it's only possible to activate integration via software upgrade

3. (This may be my hazy understading of timestamping integration, so correct me if I am wrong 😅 ) If we disable proofs generation, wouldn't be need to backfill those in upgrade eitherway ?

We don't need to. This is because the consumer side has the concept of activation as well: upon the 1st BTC timestamp the BTC timestamping integration is activated. Let's say we activate integration in Babylon, and before that the chain acts as if no consumer chain is doing integration. Upon activating integration Babylon starts generating proofs, and upon a new IBC channel Babylon starts pushing BTC timestamps to the consumer chain. The consumer chain will handle it correctly.

What your opinion on having EnableIntegration flag hardcoded in binary at app.go level ? Then:

• we do not need upgrade
• there is not way to enable it without releasing new version
• for ibc messages we can disable routing at app.go level which seems like safer option i.e instead of:

	ibcRouter := porttypes.NewRouter().
		AddRoute(ibctransfertypes.ModuleName, transferStack).
		AddRoute(zctypes.ModuleName, zoneConciergeStack).
		AddRoute(wasmtypes.ModuleName, wasmStack)

have:

	ibcRouter := porttypes.NewRouter().
		AddRoute(ibctransfertypes.ModuleName, transferStack).
		AddRoute(wasmtypes.ModuleName, wasmStack)


	if !EnableIntegration {
		ibcRouter = ibcRouter.AddRoute(zctypes.ModuleName, zoneConciergeStack)
	}

Good point on removing zone concierge from IBC router, will address later. Re having EnableIntegration hardcoded, how about having it like a genesis parameter? Similarly one can activate integration only via software upgrade, but all the feature gate stuff is scoped under x/zoneconcierge, which seems cleaner.

[KonradStaniec]

Re having EnableIntegration hardcoded, how about having it like a genesis parameter?

hmm genesis state of zoneconcierge is already fixed on TGE chain and does not have this parameter. So I imagine equivalent of it, would be:

• either to write EnableIntegration bool to zoneconcierge module storage in the upgrade, and in phase-2 upgrade write it to false and later
• or implement function on zoneconcierge keeper which tries to read this flag from storage (under some new key) and the lack of it is treated like false. Then we can use this function across the code base to gate the feature, and then on phase-3 upgrade just write true to storage. This approach will most possibly not require any upgrades/proto changes.

[SebastianElvis]

Good point. Since genesis params are not expected to be changed, maybe it's not a good idea to touch it via software upgrade.

Compared to having one more storage for it, maybe having a public bool variable in app.go is more lightweight and cleaner? This achieves the same thing, does not require us to wire this storage in upgrade handlers, and allows us to enable testing integration stuff using tricks like https://github.com/babylonlabs-io/babylon/blob/7315714451e741fb9b92f92e0f5446f671400c70/app/e2e_include_upgrades.go. Wdyt?

[KonradStaniec]

Compared to having one more storage for it, maybe having a public bool variable in app.go is more lightweight and cleaner? This achieves the same thing, does not require us to wire this storage in upgrade handlers, and allows us to enable testing integration stuff using tricks like https://github.com/babylonlabs-io/babylon/blob/7315714451e741fb9b92f92e0f5446f671400c70/app/e2e_include_upgrades.go. Wdyt?

yep hardcoding constant EnableIntegration is probably easiest and cleanest way to feature gate this whole module.

[SebastianElvis]

OK, modified this PoC to define a global boolean variable var EnableIntegration = false as feature gate, and gated the inclusion of zone concierge in IBC router. Seems to be much cleaner now.

Tried to turn on var EnableIntegration = true and run tests, and all existing tests work as expected

[SebastianElvis]

I have updated this PR according to the comments, including:

• feature-gating the GRPC queries and messages in Zone Concierge module
• enabling integration for test environments so that all existing tests still work
• adding a new fuzz test for ensuring the correct behaviours of {IBC, PostHandler, queries, message servers} when the feature gate is true/false

Please feel free to take a look again. Let me know if you want to see an end-to-end test.

[SebastianElvis]

Closing this PR as we decided to remove zone concierge for now, and re-enable later