babylonchain / babylon

Main repo for Babylon full node
https://babylonchain.io
Other
230 stars 153 forks source link

feat: BTC multisig use `sdk.AccAddress` #657

Closed RafilxTenfen closed 1 month ago

RafilxTenfen commented 1 month ago

Ref https://github.com/babylonchain/pm/pull/38

Other PR's will be created to

SebastianElvis commented 1 month ago

Thanks for the changes. One high-level comment is that I feel this PR does not complete the whole feature described in ADR-020, which seems to require two more PRs. So I would expect a base branch like this to collect the whole feature set before merging it to dev. This way, this PR is part of the feature and can be renamed to something like ADR-020: Replace Babylon Pubkey with Staker Address, ref.

Hmm given that this PR is rather self contained (unlike the pub rand commit one where we cannot stay on the status after reverting BIP32 for long), probably this PR is fine to be merged to dev?

KonradStaniec commented 1 month ago

One high-level comment is that I feel this PR does not complete the whole feature described in ADR-020, which seems to require two more PRs

From what I see, the only two prs missing are:

If thats all imo we can merge it to dev.

Although lets wait with merging this at least till core meeting. My main concern is, that we would like to return to running devnets, and if we merge it now we would need:

gitferry commented 1 month ago

@SebastianElvis @KonradStaniec Makes sense. I thought we also needed to change pop in MsgCreateFinalityProvider, but I guess that is independent of this PR. Let's merge this PR to dev then when we are ready

RafilxTenfen commented 1 month ago

For the BTC staker, I will make a PR after the resolution of new requirements for BTC delegation creation on someone's behalf