feat(adr-26): Separate inclusion proof from staking tx info #111
Conversation
gitferry
force-pushed
the
gai/adr-26-separate-inclusion-proof
branch
from
6615021
to
ba5433e
Compare
gitferry
requested review from
KonradStaniec,
Lazar955,
SebastianElvis and
RafilxTenfen
| } | ||
| } | ||
|
|
||
| message InclusionProof { |
There was a problem hiding this comment.
Message needs doc comments as otherwise make proto-lint won't pass.
A bit tangent, I am not sure how make proto-lint got out of CI, most probably during migration to re-usable pipelines.
| @@ -55,6 +55,8 @@ message Params { | |||
| ]; | |||
| // max_active_finality_providers is the maximum number of active finality providers in the BTC staking protocol | |||
| uint32 max_active_finality_providers = 13; | |||
| // base gas fee for delegation creation in Satoshi | |||
| int64 delegation_creation_base_gas_fee_sat = 14; | |||
There was a problem hiding this comment.
imo this should be an uint64 as negative value do not make sense here and we also avoid type conversion in ConsumeGas
There was a problem hiding this comment.
I thought we should keep consistent with other parameters e.g., min_slashing_tx_fee_sat and unbonding_fee_sat
There was a problem hiding this comment.
ahh I see confusion!
This should be delegation_creation_base_gas_fee , not delegation_creation_base_gas_fee_sat i.e the additional gas fee is not in satoshis but in cosmos gas unit.
min_slashing_tx_fee_sat and unbonding_fee_sat are used as int64 becouse thats what btcd library uses, so in those cases minimize amount of type conversions/
x/btcstaking/keeper/msg_server.go
Outdated
| endHeight = startHeight + uint64(parsedMsg.StakingTime) | ||
| } else { | ||
| // consume base gas fee to compensate future submission of inclusion proof | ||
| ctx.GasMeter().ConsumeGas(uint64(ms.GetParams(ctx).DelegationCreationBaseGasFeeSat), "delegation creation fee") |
There was a problem hiding this comment.
params are already retrieved in line:
// 5. Validate parsed message against parameters
vp := ms.GetParamsWithVersion(ctx)
so lets avoid making another roundtrip to database
| ) (*ParsedProofOfInclusion, error) { | ||
| if info == nil { | ||
| return nil, fmt.Errorf("cannot parse nil *btcckpttypes.TransactionInfo") | ||
| if ip == nil { |
There was a problem hiding this comment.
I think we need to allow nil inclusion proof as now this proof is optional in MsgCreateDelegation
Also in the struct ParsedCreateDelegationMessage I would leave doc comment for StakingTxProofOfInclusion field that, so something along the lines:
// StakingTxInclusionProof is optional is and it is up to the caller to verify
// whether it is present or not
StakingTxProofOfInclusion *ParsedProofOfInclusion
x/btcstaking/client/cli/tx.go
Outdated
| return err | ||
| } | ||
|
|
||
| inclusionProof, err := types.NewInclusionProofFromHex(args[3]) |
There was a problem hiding this comment.
shouldn't we call the types.NewInclusionProofFromHex( only if len(args[3]) > 0 i.e if user provides empty string as inclusion proof we create message where inclusion proof is nil ?
x/btcstaking/keeper/msg_server.go
Outdated
| startHeight = inclusionHeight | ||
| endHeight = startHeight + uint64(parsedMsg.StakingTime) | ||
| } else { | ||
| // consume base gas fee to compensate future submission of inclusion proof |
There was a problem hiding this comment.
hmm this is a bit misleading, I would say that we consume more gas to protect babylon chain and covenant members against spamming i.e creating delegation that will never reach btc chain
ref.
This PR implements the separates the handling inclusion proof from btc delegation. In particular, this PR:
delegation_creation_base_gas_fee_satwhich will be used to consume more gas when the inclusion proof is not included in the delegationA subsequent PR will introduce a new message to handle the inclusion proof