Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt spend stake endpoint to unbonding tx #48

Merged
merged 3 commits into from
Sep 18, 2023

Conversation

KonradStaniec
Copy link
Collaborator

Changes:

  • rename endpoint from spend_staking_tx to spend_stake
  • make spend_stake endpoint handle the case when stake was unbonded i.e it is currently in unbonding output.

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the generalization!

Comment on lines 185 to 186
foundingOutput *wire.TxOut
foundingOutputScript []byte
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
foundingOutput *wire.TxOut
foundingOutputScript []byte
fundingOutput *wire.TxOut
fundingOutputScript []byte

@@ -197,7 +204,7 @@ const (

// after this many confirmations we consider transaction which spends staking tx as
// confirmed on btc
stakingTxSpendTxConfirmation = 3
SpendStakeTxConfirmations = 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still think that those should come from Babylon, see my comment here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main qualm about tying it to babylon is that not always what is good for solo staker, is good for the chain as chain may require larger security margins i.e it is not entirely impossible that in some time we will say that k should be 20 to avoid thinking about any re-org possiblities or maybe we will delete k and leave only w . The those values will definitely be too large for any solo staker preferences.

I agree that current way is a bit haphazard, and using different values everywhere is a no-go for the future. My current thinking goes to introducing SafetyConfirmation parameter in config, which will have some minimum value like 2, and using it everywhere where does value does not depend on babylon safety. wdyt ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now created issue for that we can discuss it further there - #49

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm ok, np with that. We should be clear that this is different than Babylon's parameter though and on some occassions the Babylon parameter is gonna be used.

Comment on lines 1982 to 1985
foundingTx *wire.MsgTx,
foundingTxHash *chainhash.Hash,
foundingTxStakingScript []byte,
foundingTxStakingOutputIdx uint32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
foundingTx *wire.MsgTx,
foundingTxHash *chainhash.Hash,
foundingTxStakingScript []byte,
foundingTxStakingOutputIdx uint32,
fundingTx *wire.MsgTx,
fundingTxHash *chainhash.Hash,
fundingTxStakingScript []byte,
fundingTxStakingOutputIdx uint32,

Copy link

@gitferry gitferry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

// - transaction is in state SENT_TO_BABYLON, i.e staker created valid delegation and sent it to babylon
// - transaction is in state UNBONDING_CONFIRMED_ON_BTC, i.e staker already sent unbonding tx to btc network and
// it is confirmed
func (s *StoredTransaction) IsSpendableOnBtc() bool {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ever be called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh leftover I will delete it.

@KonradStaniec
Copy link
Collaborator Author

KonradStaniec commented Sep 18, 2023

marked pr as breaking due to api endpoint name change

@KonradStaniec KonradStaniec merged commit 7fe9aef into main Sep 18, 2023
2 checks passed
@KonradStaniec KonradStaniec deleted the hande-unstaking-unbonded-funds branch September 18, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants