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

Create a flyover redeem script builder #2727

Merged
merged 6 commits into from
Sep 17, 2024
Merged

Conversation

marcos-iov
Copy link
Contributor

Description

Create a flyover redeem script builder to be used in substitution of the one provided by bitcoinj-thin. Looking to decouple Bridge related logic from bitcoinj-thin

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)
  • Other information:

Remove unused import

Remove mistaken reason

Minor refactor

Make flyover rs builder receive a keccak256 hash instead of sha256 one

Refactors
@marcos-iov marcos-iov requested a review from a team as a code owner September 13, 2024 15:45
julia-zack
julia-zack previously approved these changes Sep 13, 2024

@ParameterizedTest
@MethodSource("invalidDerivationHashArgsProvider")
void addFlyoverDerivationHashToRedeemScript_withInvalidPrefix_shouldThrowFlyoverRedeemScriptCreationException(Keccak256 flyoverDerivationHash) {
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about improving test name adding whenInvalidPrefix?:

addFlyoverDerivationHashToRedeemScript_whenInvalidPrefix_shouldThrowFlyoverRedeemScriptCreationException

}

@Test
void addFlyoverDerivationHashToRedeemScript_shouldReturnRedeemScriptWithFlyoverDerivationHash() {
Copy link
Contributor

Choose a reason for hiding this comment

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

whenValidFlyoverDerivationHash? WDYT?:

addFlyoverDerivationHashToRedeemScript_whenValidFlyoverDerivationHash_shouldReturnRedeemScriptWithFlyoverDerivationHash

nathanieliov
nathanieliov previously approved these changes Sep 16, 2024
Copy link
Contributor

@nathanieliov nathanieliov left a comment

Choose a reason for hiding this comment

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

Just a name suggestion

import co.rsk.bitcoinj.script.Script;
import co.rsk.crypto.Keccak256;

public interface FlyoverRedeemScriptBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why merge it into the master branch instead of the integration branch, wip/fed-scripts-refactors-integration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes it easier to integrate all the changes we are working on in different branches

Copy link

sonarcloud bot commented Sep 17, 2024

@josedahlquist josedahlquist merged commit 0994382 into master Sep 17, 2024
10 checks passed
@marcos-iov marcos-iov deleted the flyover-rs-parser branch September 17, 2024 15:41
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.

5 participants