Fix Issues After Remove createErpRedeemScript method from ErpFederationRedeemScriptParser in Bj-thin #2729
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reorder rskip
Add tests Refactor Rename variable
Add key to storage index Remove declaration of svp fund tx hash since it is not being used yet. Remove _KEY suffix
Remove _KEY suffix. Save proposed federation the same way as pending one
Add proposedFederationIsSet logic Getting rid of proposedFederationIsSet logic. Add isProposedFederationSet logic. Add rskip419 check in saveProposedFederation method Save null version when proposed federation is null Remove unnecessary private method Refactor Removes null activations from tests
Make variable final Improve test name Use all activations instead of lovell Improve null handling
Add get proposed federation tests Minor fix after rebase Improve comment Minor refactor Minor refactor Improve comment Add test case Throw exception when there is no storage version for non-null proposed federation Add test cases Add log. Add test case and refactor Remove unused import
…sh methods and tests Rebases Reorders and renames tests Rebases Appends SVP prefix to FUND_TX_HASH_UNSIGNED and refactors tests Rebases Moves repeated arrange code to setup. Using Optional. Rebases Removes _ from the key Using arrowhead631 Rebases Rebases Renames saveFundTransactionUnsignedHash to saveSvpFundTransactionUnsignedHash Renames FundTransactionUnsignedHash instance fields
Remove unnecessary private method Refactor
Add get proposed federation tests Minor fix after rebase Improve comment Minor refactor Add test case
…d federation Add test cases Add log. Add test case and refactor Remove unused import
…sh methods and tests Rebases Moves repeated arrange code to setup. Using Optional. Rebases
Adds test to assert empty is returned when hash hasn't been set or saved Renames svp fields to match the RSKIP419 description. Adds more tests. Using standard. Renames test
Improve variable name Add comment Minor refactors Improve comments. Minor refactors Improve comments Refactor Add tests for commitFederationAccordingToActivations Remove unnecessary semicolon Refactor to improve testing and readability Add test cases Remove some tests. Make bridge event logger not a mock. Make some methods private instead of protected. Fix log message Improve test name Change test name Move reused method to utility class. Use real block instead of a mock Make methods to be private instead of protected
Add method to utils
Fix sonar complains Add missing import Move method to test class Remove unused imports
…y to bridge storage index key
… to bridge storage provider Remove svp fund tx tests from federation storage provider tests, and adds them to bridge storage provider tests Get rid of sonar complains Add get methods to bridge Put save, set and get tests in same nested class
Add tests. Add missing tests for minimum pegout tx value Modify test
Remove unused import Remove mistaken reason Minor refactor Make flyover rs builder receive a keccak256 hash instead of sha256 one Refactors
…o RedeemScriptCreationException
… creating pegouts
Refactor and add test case Add test case Remove mistaken comment Add comment Modify comment
Rename variables Modify method to just remove signatures from non-segwit transactions Modify message from thrown exception to be more accurate Add indentation to improve readability
Remove unnecessary import resolve comments
… inputs or without inputs with p2sh multisig script sig. Rename. Reorder blank lines Rename scriptSig to inputScript Set network parameters at bridge support class level to avoid duplication code
…of updateScriptWithSignature Add [methodName] when logging. Use getSigInsertionIndex to make method more realistic. Remove error being thrown when tx has no inputs. Remove unused import
…ve methods naming Fix wrong log
…using mocks Refactor and renaming
…action Improve comments Improve method name. Add transactionsInBlock argument for creating a pmt. Improve method to create a valid pmt for transactions following class implementation
Once the method signature on RedeemScriptParser#extractStandardRedeemScript is changed, we need to apply this to RSKj to modify how the method is used from it. Old method signature: Script extractStandardRedeemScript(); New method signature: List<ScriptChunk> extractStandardRedeemScriptChunks();
…hod inside the parsers - Fix MultiSigType.ERP_FED reference to MultiSigType.NON_STANDARD_ERP_FED after refactor. - Update isPegOutTx and isValidPegInTx instead of passing script sig, pass redeem script chunks to RedeemScriptParserFactory.get.
marcos-iov
reviewed
Sep 16, 2024
| @@ -155,4 +157,23 @@ private static void signLegacyTransactionInputFromP2shMultiSig(BtcTransaction tr | |||
| input.setScriptSig(inputScriptSig); | |||
| } | |||
| } | |||
|
|
|||
| public static List<BtcECKey> getDefaultPublicKeys() { | |||
There was a problem hiding this comment.
What's the use for this? Seems to be very specific
There was a problem hiding this comment.
This method gets the default public keys. It'll be use only for tests. Should it be in a different Utils Class?
There was a problem hiding this comment.
Done! Started using the getBtcPublicKeys method from the active federation. Reversed the changes on BitcoinTestUtils.
…eem-script-class Update ErpRedeemScriptParser reference to new name and fix tests failing after RedeemScriptParserFactory refactor
…onRedeemScriptParser in Bj-thin After removing the createErpRedeemScript method from ErpFederationRedeemScriptParser in Bitcoinj-thin, please take a look at RSKj to see the impact this has on it. Fix classes using the createErpRedeemScript method from ErpFederationRedeemScriptParser using the redeem script builder in RSKj.
wilmerrootstock
force-pushed
the
feature/remove-createerpredeemscript-from-bjthin
branch
from
c9f1e9c to
0ee2d22
Compare
| Federation activeFederation = FederationTestUtils.getGenesisFederation(federationConstantsMainnet); | ||
| int defaultThreshold = activeFederation.getBtcPublicKeys().size() / 2 + 1; | ||
| int emergencyThreshold = federationConstantsMainnet.getErpFedPubKeysList().size() / 2 + 1; | ||
| ErpRedeemScriptBuilder nonStandardErpRedeemScriptBuilder = new NonStandardErpRedeemScriptBuilder(); |
There was a problem hiding this comment.
Please use NonStandardErpRedeemScriptBuilderFactory instead.
|
@wilmerrootstock, these tests failing were already fixed in this PR: #2730 by me |
nathanieliov
force-pushed
the
wip/fed-scripts-refactors-integration
branch
from
38cdfcd to
4b55c0d
Compare
|
The task was addressed by @nathanieliov when he was fixing some issues on RSKj in this PR |
marcos-iov
deleted the
feature/remove-createerpredeemscript-from-bjthin
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
After removing the
createErpRedeemScriptmethod fromErpFederationRedeemScriptParserinBitcoinj-thin, please take a look atRSKjto see the impact this has on it.Fix classes using the
createErpRedeemScriptmethod fromErpFederationRedeemScriptParserusing the redeem script builder inRSKj.Motivation and Context
This relates to the Refactors to add
SegwitRedeemScriptParser.How Has This Been Tested?
Unit Tests.
Types of changes
Checklist: