rafactor/Refactor handler for staker message #33
Conversation
x/btcstaking/keeper/msg_server.go
Outdated
| // 2. Basic stateless checks | ||
| // - verify proof of possession | ||
| if err := parsedMsg.ParsedPop.Verify(parsedMsg.StakerAddress, parsedMsg.StakerPK.BbnPk, ms.btcNet); err != nil { | ||
| return nil, types.ErrInvalidProofOfPossession.Wrapf("error while validating proof of posession: %v", err) |
There was a problem hiding this comment.
| return nil, types.ErrInvalidProofOfPossession.Wrapf("error while validating proof of posession: %v", err) | |
| return nil, types.ErrInvalidProofOfPossession.Wrapf(err) |
The message is already in the error text
x/btcstaking/keeper/msg_server.go
Outdated
|
|
||
| // 4. Check finality providers to which message delegate | ||
| // Ensure all finality providers are known to Babylon, are not slashed, | ||
| // and their registered epochs are finalised |
There was a problem hiding this comment.
we should cut the last line as we don't have the registered epoch anymore
x/btcstaking/keeper/msg_server.go
Outdated
| } | ||
| // ensure the finality provider is not slashed | ||
| if fp.IsSlashed() { | ||
| return nil, types.ErrFpAlreadySlashed |
There was a problem hiding this comment.
how about wrapping the slashed fp pk?
x/btcstaking/keeper/msg_server.go
Outdated
| } | ||
|
|
||
| // no need to do more validations to the btc header as it was already | ||
| // validate by the btclight client module |
There was a problem hiding this comment.
| // validate by the btclight client module | |
| // validated by the btclightclient module |
| endHeight := stakingTxHeader.Height + uint64(parsedMsg.StakingTime) | ||
|
|
||
| btcTip := ms.btclcKeeper.GetTipInfo(ctx) | ||
| stakingTxDepth := btcTip.Height - stakingTxHeader.Height |
There was a problem hiding this comment.
| stakingTxDepth := btcTip.Height - stakingTxHeader.Height | |
| stakingTxDepth := btcTip.Height - stakingTxHeader.Height + 1 |
Should we follow the convention that inclusion counts as 1 confirmation?
There was a problem hiding this comment.
I think on Babylon node we are using depth everywhere i.e for being k-deep, and for w-deep. Of course we may discuss this change as it makes sense to be compatible with phase-1, but this is not something to be done in refactor pr.
For now I have created ticket to track this - #39
x/btcstaking/keeper/msg_server.go
Outdated
| type ParamsValidationResult struct { | ||
| StakingOutputIdx uint32 | ||
| UnbondingOutputIdx uint32 | ||
| } | ||
|
|
||
| // ValidateParams validates parsed message against parameters | ||
| func ValidateParams( |
There was a problem hiding this comment.
What do you think we move the params-related code to a separate file or a different package?
And would it be better if we make ValidateParams a method of ParsedCreateDelegationMessage?
There was a problem hiding this comment.
hmm I guess it can be moved to types as it really do not touch/modify the state.
As for making it a method on ParsedCreateDelegationMessage, I am slightly against mostly due to the fact validating against the parameters is not really a property of ParsedCreateDelegationMessage (I know this sounds weird 😅 ) but rather operation that is by using ParsedCreateDelegationMessage with other pieces of data.
Maybe in other words, ParsedCreateDelegationMessage is just data transfer object without any assumptions how it will be used. Other functions, object by use it for its own purposes, but the dto object should be as bare as possible.
| pk, err := key.ToBTCPK() | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parer *bbn.BIP340PubKey: %w", err) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("failed to parer *bbn.BIP340PubKey: %w", err) | |
| return nil, fmt.Errorf("failed to parse *bbn.BIP340PubKey: %w", err) |
| PublicKey *btcec.PublicKey | ||
| BbnPk *bbn.BIP340PubKey |
There was a problem hiding this comment.
| PublicKey *btcec.PublicKey | |
| BbnPk *bbn.BIP340PubKey | |
| *btcec.PublicKey | |
| *bbn.BIP340PubKey |
A bit confusing about BbnPk because we also call it a BTC public key
| Signature *schnorr.Signature | ||
| BbnSig *bbn.BIP340Signature |
There was a problem hiding this comment.
| Signature *schnorr.Signature | |
| BbnSig *bbn.BIP340Signature | |
| *schnorr.Signature | |
| *bbn.BIP340Signature |
Same here. BbnSig is a bit confusing
KonradStaniec
changed the title
Refactor our staking handler.
Pre-requisite before implementing: https://github.com/babylonlabs-io/pm/blob/main/adr/adr-023-update-btcstaking-params.md