feat: utilities for instantiating Babylon contracts #66
Conversation
SebastianElvis
changed the title
| @@ -17,6 +17,8 @@ | |||
| - [Query](#babylonlabs.babylon.v1beta1.Query) | |||
|
|
|||
| - [babylonlabs/babylon/v1beta1/tx.proto](#babylonlabs/babylon/v1beta1/tx.proto) | |||
| - [MsgInstantiateBabylonContracts](#babylonlabs.babylon.v1beta1.MsgInstantiateBabylonContracts) | |||
There was a problem hiding this comment.
I think providing a message for instantiation is not without merit, but why not just instantiate the contracts during bootstrapping?
Since the contracts are fundamental for the SDK operation, I don't see the need to gate their instantiation through a specific protobuf endpoint.
There was a problem hiding this comment.
why not just instantiate the contracts during bootstrapping?
I was thinking about this as well, but decided to not do this (at least for this PR):
- This makes it mandatory to have these wasm binaries in hand when executiong
babylondto start the chain. Otherwise we would need to somehow wire the wasm binaries into thebabylondbinary, which is not ideal. - It's possible that one wants to invoke
MsgInstantiateBabylonContractsafter the chain has been running for a while, e.g.,- the chain has just passed a gov prop for integrating with Babylon;
- the chain does not want the old babylon contracts anymore, and wants to deploy a new set of babylon contracts, e.g., due to corrupted DB state
- Even if we want to instantiate all contracts upon starting the chain, this could be done in a separate PR. This separate PR basically executes the same logic of
MsgInstantiateBabylonContractshandler upon starting the chain. Other things are not affected.
Wdyt?
There was a problem hiding this comment.
OK. But please notice that regarding
This makes it mandatory to have these wasm binaries in hand when executiong babylond to start the chain.
these contracts are on the Consumer side, and so, they are not mandatory for Babylon.
Moreover, there's a way to embed the contracts inside the binary, which makes it pretty much stand-alone. See
for reference / as an example.
There was a problem hiding this comment.
Looks good, but I have some concerns with the overall architecture.
This also seems to be in conflict with the changes in (draft) babylonlabs-io/babylon-contract#94.
Let's discuss offline.
| } | ||
| if len(config.BTCFinality) == 0 { | ||
| return "", "", "", errorsmod.Wrap(types.ErrInvalid, "failed to instantiate BTC finality contract") | ||
| } |
There was a problem hiding this comment.
As I understand it, you're still delegating the instantiation of the staking and finality contracts to the babylon contract here.
I would very much "flatten" this. Making this function to instantiate all the contracts explicitly, and passing their addresses along, as detailed in #45.
| "btc_confirmation_depth": btcConfirmationDepth, | ||
| "checkpoint_finalization_timeout": checkpointFinalizationTimeout, | ||
| "notify_cosmos_zone": notifyCosmosZone, | ||
| "btc_staking_code_id": btcStakingCodeId, |
There was a problem hiding this comment.
Instead of passing the code ids and relying on the internal instantiation logic, let's just flatten this for the SDK to instantiate everything explicitly.
WDYT? Let's discuss offline in any case.
This PR provides a message
MsgInstantiateBabylonContractsfor instantiating all Babylon contracts. This simplifies the Cosmos integration: one can first store code of 3 contracts, and then submit this message for instantiating everything.With this message as baseline, we can then change the handler logic to improve the instantiation flow, e.g., instantiate 3 contracts one by one in this handler, rather than allowing contracts instantiating each other. This will be done in a subsequent PR.
The PR contains the following: