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

fix: pre approval flow fixes #65

Merged
merged 7 commits into from
Oct 16, 2024
Merged

Conversation

KonradStaniec
Copy link
Collaborator

  • Separates staker commands from events
  • Fix sending signed transaction to Babylon
  • Remove unnecessary state (CREATED) to make sure staking command returns to caller only if staking tx was delivered either to Babylon or to BTC

@KonradStaniec KonradStaniec force-pushed the konradstaniec/pre-approval-fixes branch from 0652e9f to 49640ac Compare October 15, 2024 15:34
Copy link
Contributor

@RafilxTenfen RafilxTenfen left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Member

@Lazar955 Lazar955 left a comment

Choose a reason for hiding this comment

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

Nice work! Some small nits, can ignore if you disagree

stakingOutputIndex uint32,
stakingTime uint16,
fpPubKeys []*btcec.PublicKey,
pop *ProofOfPossession,
Copy link
Member

Choose a reason for hiding this comment

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

might be useful to check pop is not nil to avoid nil pointer

return nil, fmt.Errorf("cannot add transaction without finality providers public keys")
}

var fpPubKeysBytes [][]byte = make([][]byte, len(fpPubKeys))
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
var fpPubKeysBytes [][]byte = make([][]byte, len(fpPubKeys))
fpPubKeysBytes := make([][]byte, len(fpPubKeys))

No need to be redundant with type declaration

}

return c.addTransactionInternal(
txHashBytes, &msg, nil,
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
txHashBytes, &msg, nil,
txHash[:], &msg, nil,

serializedTx, err := utils.SerializeBtcTransaction(btcTx)

if err != nil {
return err
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
return err
return fmt.Errorf("failed to serialize Bitcoin transaction: %w", err)

update, err := newInitialUnbondingTxData(unbondingTx, unbondingTime)

if err != nil {
return err
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
return err
return fmt.Errorf("failed to create unbonding transaction data: %w", err)

@KonradStaniec KonradStaniec merged commit 16c8dd0 into main Oct 16, 2024
12 checks passed
@KonradStaniec KonradStaniec deleted the konradstaniec/pre-approval-fixes branch October 16, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants