Whitelist Refactor Integration #2545
Merged
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import org.junit.jupiter.params.provider.Arguments; | ||
| import org.junit.jupiter.params.provider.MethodSource; | ||
|
|
||
| class WhitelistConstantsTest { |
Check notice
Code scanning / CodeQL
Unused classes and interfaces Note test
| Sha256Hash merkleRoot = pmt.getTxnHashAndMerkleRoot(new ArrayList<>()); | ||
| co.rsk.bitcoinj.core.BtcBlock btcBlock = | ||
| new co.rsk.bitcoinj.core.BtcBlock(bridgeConstantsRegtest.getBtcParams(), 1, PegTestUtils.createHash(), merkleRoot, | ||
| new co.rsk.bitcoinj.core.BtcBlock(bridgeMainNetConstants.getBtcParams(), 1, PegTestUtils.createHash(), merkleRoot, |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| Sha256Hash merkleRoot = pmt.getTxnHashAndMerkleRoot(new ArrayList<>()); | ||
| co.rsk.bitcoinj.core.BtcBlock btcBlock = | ||
| new co.rsk.bitcoinj.core.BtcBlock(bridgeConstantsRegtest.getBtcParams(), 1, PegTestUtils.createHash(), merkleRoot, | ||
| new co.rsk.bitcoinj.core.BtcBlock(bridgeMainNetConstants.getBtcParams(), 1, PegTestUtils.createHash(), merkleRoot, |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
marcos-iov
force-pushed
the
wip/whitelist-refactor-integration
branch
from
f4b869a to
5dc3a30
Compare
|
pipeline:run |
1 similar comment
|
pipeline:run |
rskj-core/src/test/java/co/rsk/peg/BridgeTestIntegration.java
Dismissed
Show dismissed
Hide dismissed
rskj-core/src/test/java/co/rsk/peg/BridgeTestIntegration.java
Dismissed
Show dismissed
Hide dismissed
rskj-core/src/test/java/co/rsk/peg/BridgeTestIntegration.java
Dismissed
Show dismissed
Hide dismissed
marcos-iov
force-pushed
the
wip/whitelist-refactor-integration
branch
from
76d3a70 to
cda9adb
Compare
Create an interface WhitelistStorageProvider under co.rsk.peg.whitelist package. Include the signature of the 2 methods Whitelist-related from BridgeStorageProvider. Addressing Review Comments - Remove new line. - Refactoring save() name method.
New enum WhitelistResponseCode that contains the response codes for Whitelist. Currently, they are part of BridgeSupport, we are going to remove them, and put all in the new object. Remove LOCK_WHITELIST prefix, and _CODE suffix since don't add any value. Addressing Review Comments - Remove new blank line. - Refactoring constant names from WhitelistResponseCode enum.
Create an interface called WhitelistSupport and put all the method signatures Whitelist-related on it. Take these methods from BridgeSupport. The plan is to remove all of them from BridgeSupport into their own WhitelistSupport class. Whitelist Support Interface - Adding JavaDoc. Changing Variable Types On Return Value - Replacing Integer Wrapper Class With int primitive data type to avoid null values.
- Following with the Bridge refactors, it is now time to refactor and remove whitelisting logic from BridgeConstants class. Removing an extra Blank Line - Addressing review comment related to Remove an extra Blank Line.
Create an implementation of WhitelistStorageProvider under co.rsk.peg.whitelist package. Include the implementation of the 2 methods Whitelist-related from BridgeStorageProvider that save the Whitelist to the storage and the method that get the lock Whitelist. Removed Additional Changes to Avoid Errors - Removed Additional Changes to Avoid Errors since they are not necessary because BridgeStorageProvider is going to be removed in a future as part of Bridge refactor. Renaming Variable - Renaming variable from storageAccessor to bridgeStorageAccessor. Applying Good Practices For Lazy Initialization - Applying Good Practices for Lazy Initialization Methods, like synchronized the method where it is. -Moving logic to initialize a variable to a private method to improve readability. - Improving some indentation.
Multiple storage keys are all together included in BridgeStorageIndexKey Enum. Now that we are refactoring Whitelist logic to its own package and classes, we should also crete its own Enum with storage index keys. Create a new enum called WhitelistStorageIndexKey under co.rsk.peg.whitelist package. Move from BridgeStorageIndexKey to WhitelistStorageIndexKey, and make the respective refactor related.
Following the Bridge refactors, we already migrated the WhitelistConstants to their classes to break the high coupling Whitelist had with the Bridge objects, however, it’s missing add coverage, so we need to create Unit Tests to get reliability your new code doesn't break existing functionality when a change is made to the application. Create Test Cases for all Networks such as MainNet, TestNet, and RegTest. Addressing Review Comments - Improving WhitelistConstants: test and production code for all Networks.
Create the implementation of the interface called WhitelistSupport, and put all the method Whitelist-related on it. Take these methods from BridgeSupport. The plan is to remove all of them from BridgeSupport into their own WhitelistSupport class. This implementation class contains all the business logic related to Whitelist. JavaDoc for WhitelistSupportImpl. - Adding JavaDoc to document WhitelistSupportImpl. Addressing Review Comments - Moved verifyLockSenderIsWhitelisted method from BridgeSupport to WhitelistSupport. - UNAUTHORIZED_CALLER constant added to WhitelistResponseCode. - Replaced GENERIC_ERROR with UNAUTHORIZED_CALLER. - Removed PanicProcessor. - added some custom indentation to WhitelistSupportImpl constructor. - Removed whitelist prefix to variables. - isAuthorizedLockWhitelistChange changed name to isNotAuthorizedLockWhitelistChange. This is a IntelliJ suggestion to make more readable the code. -Added logger for AddressFormatException in removeLockWhitelistAddress method. isAuthorizedLockWhitelistChange method refactoring - Reverted improvement since at this stage it is not needed. The improvement was made based on avoiding negation statements into conditionals to make the code more readable. - Refactor reverted to leave it like it was before refactoring. Addressing Review Comments - Moving method JavaDocs from WhitelistSupportImpl implementation class to WhitelistSupport interface. - Removing Class JavaDoc on WhitelistSupportImpl implementation class since it doesn't add much value. - Improving logger messages to align to standard. - Removing "this" keyword since in these cases it doesn't add much value. - Removing generic Exception, IOException, BlockStoreException. They are since they are not thrown in any place. - Removing comments in verifyLockSenderIsWhitelisted. These comments are out of place. - Removed UNKNOWN_ERROR in WhitelistResponseCode. Moving addLockWhitelistAddress Method - To improve readability, we moved the addLockWhitelistAddress private method, right next to the public methods where it is being used. Refactoring Method Name Update WhitelistSupport javadocs Replace btcContext param for NetworkParameters in WhitelistSupportImpl Remove javadoc from whitelist related methods in BridgeSupport Remove unused field from BridgeDevNetConstants
Remove network parameters from WhitelistSupport, get the value from WhitelistConstants
Add Unit Tests for WhitelistSupportImpl making sure we are simulating real scenarios. Renaming Test Methods - Renaming Test Methods to get closer what they do. Add UTs for verifyLockSenderIsWhitelisted in WhitelistSupportImplTest As part of Bridge refactors, this is time to work on Whitelist process, so we need to extract this from the Bridge and create its own objects to decouple the process. Create UTs for verifyLockSenderIsWhitelisted to increase coverage on WhitelistSupportImplTest. Verify address as consumed after verification
Create a new class WhitelistStorageProviderImplTest, look for test cases that make use of Whitelist related methods in BridgeStorageProviderTest and move those tests to the new class. WhitelistStorageProviderImplTest New Unit Tests created. The old tests were removed since we are reducing the use of mocks to get more realistic UTs. Improving The Tests - Improving the tests to get them more realistic how Whitelist process set OneOffWhiteListEntry and UnlimitedWhiteListEntry in LockWhitelist. Adding saveInMemoryStorageOneOffWhiteListEntry method - Adding saveInMemoryStorageOneOffWhiteListEntry method to simulate a previous value InMemoryStorage. Checking LockWhitelist Out - Making sure that lockWhitelist is empty when we get data from it at the beginning, before saved some record InMemoryStorage. Addressing Review Comments - Created BitcoinTestUtils.getBtcAddress method to returns a BTC address derived from the seed. - Added 2 instance variables related to btcAddress to be used in the UTs. - Added new assertions to verify were saved the correct Addresses values InMemoryStorage. - Added new assertions to verify were whitelisted the correct addresses. Removing A Redundant Method From BitcoinTestUtils. - Removing a redundant Method from BitcoinTestUtils, and reusing one that already exists. Renaming Methods - Renaming methods to get closer what they do. Recreating whitelistStorageProvider - Recreating whitelistStorageProvider to make sure were saved the entries in storage. Setting a new value to maxTransferValue variable. Setting a new value to maxTransferValue variable. Renaming test method From: getLockWhitelist_whenLockWhitelistIsNull_shouldReturnZeroEntries To: getLockWhitelist_whenNoEntriesInStorage_shouldReturnZeroEntries Inserting Entries Directly In Storage For Get Methods - Inserting entries directly in storage, and after that, query those entries in storage and apply assertions. Renaming Method Related To ActiveRSKIP87 - Renaming Method Related To ActiveRSKIP87: From: void getWhitelist_whenIsActiveRSKIP87_shouldReturnTwoEntries To: getWhitelist_whenIsActiveRSKIP87_shouldReturnUnlimitedEntries Modifying Method Name - Modifying a part of a method name that was using pascal case. Changing it to camel case. Changing Return Object - Changing Return Object of getAddressFromUnlimitedWhiteListEntryInMemoryStorage method. Adding In Parameters To Method Signatures -Adding Address parameter to method Signatures of createOneOffWhiteListEntry and createUnlimitedWhiteListEntry methods. Using createOneOffWhiteListEntry and createUnlimitedWhiteListEntry - Replacing a manual process to create these two objects with the method that does it. Adding Additional Assertions to make sure Storage Is Empty Changing Method Name To Indicate RSKIP87 Is Not Active - Changing Method Name To Indicate RSKIP87 Is Not Active. - Make sure there no values in storage related to addressUnlimitedWhiteListEntryMap Improving Test Case - Changing the name to get closer what it does. - This test case is based on: 1. The first time it get values, LockWhitelist is null, and it queries the storage. 2. It should return zero entries as there is no value saved in storage. As of here LockWhitelist is not null. 3. Save a value directly in storage. 4. The second time it gets values, LockWhitelist is not null so that it doesn't query the storage and get the value from cache. 5. Return zero entries as it doesn't query in storage. 6. Recreate whitelistStorageProvider to make sure it is querying the storage. 7. Return one entry that was saved directly in storage. 8. Making sure the correct value was whitelisted Adding Whitespace Between // And The Comment. Improving Comments Fixing A Part Of Method Name with Camel Case Improving Comment Improving Name Methods Creating A Temporary Variable - Creating a temporary variable to get all Queries from getLockWhitelist. - Removing the previous variables since they do not add more value. Improving Test Case Name Fix Issues Related To Whitelist Refactor Whitelist-related logic has now been moved to its classes, however, this refactor broke some classes that we need to fix. Create a branch from the Integration Whitelist branch and fix all the test classes broken with issues related to Whitelist. Comment out a line in LockWhitelist performance test. Leave a TODO for future rework
Whitelist related logic has now been moved to its classes, however, those methods are still being called through BridgeSupport. We need to add some coverage for those methods. Create tests in BridgeSupportTest, to give coverage for the whitelist related methods that call WhitelistSupport. There’s not much logic to test there, simply mock WhitelistSupport class response and give coverage to those methods. Replacing Argument Matchers - Replacing Argument Matchers with variables, and in some cases Strings, to improve the readability of the code. Adding Coverage To setLockWhitelistDisableBlockDelay Method Removing Integer.SIZE To Make Clearer The Logic Adding @nested annotation at an inner class to improve Whitelist Tests organization into BridgeSupportTest. Adding a setup method to avoid repeat code in the tests related to Whitelist. Recreating bridgeSupport variable with all elements needed for setLockWhitelistDisableBlockDelay test to avoid NPE. Second Part Issues Related To Whitelist Refactor Whitelist-related logic has now been moved to its classes, however, this refactor broke some classes that we need to fix. Create a branch from the Integration Whitelist branch and fix all the test classes broken with issues related to Whitelist. Second Part Issues Related To Whitelist Refactor Fixing BridgeSupportTest.
Fixing issues in BridgeSupportTest - Removing a WhitelistSupport mock to replace it with an instance of the implementation. Fixing Issues in BridgeSupportTest Fixing Issues in BridgeSupportTestIntegration
Fixing Variable Standard Moving whitelistSupport variable to a better place Modifying Comments to get closer What the code does Replacing activationsBeforeForks with activations variable Replacing activationsBeforeForks with activations variable Removing Unnecessary Code Lines Replacing bridgeConstantsRegtest with bridgeMainNetConstants Removing Manually Whitelist Address Consumption Replacing activationConfig with lovell700Activations Fixing Issue Related to Whitelist save Method. Third Part Fix Issues Related To Whitelist Refactor Whitelist-related logic has now been moved to its classes, however, this refactor broke some classes that we need to fix. Create a branch from the Integration Whitelist branch and fix all the test classes broken with issues related to Whitelist. Removed getLockWhitelistMethods - Removed getLockWhitelistMethods Since This Test Case Is Covered
- Fix an issue in getBtcParams test comparing in the assertions the BtcParams in different networks. Add whitelist support to bridge support creation to fix tests
- A new save method was added to WhitelistSupport, so we must add coverage. Removing Magic Number - Removing Magic Number and replacing it with a meaningful constant. Apply ActivationConfigForTest - Rather than activating specific RSKIPs, apply ActivationConfigsForTest class, use all() so we ensure the test keeps passing with future RSKIPs.
marcos-iov
force-pushed
the
wip/whitelist-refactor-integration
branch
from
bbb9a7f to
aabcffb
Compare
marcos-iov
approved these changes
Aug 6, 2024
|
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
Following the Bridge refactor tasks, it is time to refactor and remove whitelisting logic from the Bridge.
Removed whitelisting logic from the
BridgeConstantsclass.New enum,
WhitelistResponseCode, that contains the response codes for Whitelist. Currently, they are part ofBridgeSupport, and we are removing and putting them all together in the new object. Removed theLOCK_WHITELISTprefix, and_CODEsuffix since they don't add any value.Created an interface,
WhitelistStorageProvider, under theco.rsk.peg.whitelistpackage. Include the signature of the 2 methods Whitelist-related fromBridgeStorageProvider.Created an implementation of the
WhitelistStorageProviderunder theco.rsk.peg.whitelistpackage. Include the implementation of the 2 methods Whitelist-related fromBridgeStorageProviderthat saves the Whitelist to the storage and the method that gets the lock Whitelist.Created an interface called
WhitelistSupport, and put all the method signatures Whitelist-related on it. Take these methods fromBridgeSupport. The plan is to remove all of them fromBridgeSupport, and put them into theWhitelistSupportclass.Created the implementation of the
WhitelistSupportinterface, and put all the method Whitelist-related on it. Take these methods fromBridgeSupport. The plan is to remove all of them fromBridgeSupport, and put them into theWhitelistSupportclass. This implementation class must contain all the business logic related to Whitelist.Following the Bridge refactors, we already migrated the WhitelistConstants to their classes to break the high coupling Whitelist had with the Bridge objects, however, it’s missing add coverage, so we created Unit Tests to get reliability and ensure new code doesn't break existing functionality when a change is made to the application.
Created Test Cases for all Networks such as MainNet, TestNet, and RegTest.
Due to multiple storage keys being included in
BridgeStorageIndexKeyEnum, we need to refactor theWhitelistlogic to its package and classes so we created its Enum with storage index keys.Create a new enum called
WhitelistStorageIndexKeyunder theco.rsk.peg.whitelistpackage. Move fromBridgeStorageIndexKeytoWhitelistStorageIndexKeywhitelist constants, and make the respective refactor related.Created a new class
WhitelistStorageProviderImplTest, looked for test cases that use Whitelist-related methods inBridgeStorageProviderTest, and moved those tests to the new class.Added Unit Tests for the
WhitelistSupportImpl, ensuring we are simulating real scenarios.Created UTs for
verifyLockSenderIsWhitelistedto increase coverage onWhitelistSupportImplTest.Created tests in
BridgeSupportTest, to give coverage for the whitelist related methods that callWhitelistSupport. There’s not much logic to test there, so we mock theWhitelistSupportclass response and give coverage to those methods.Whitelist-related logic has now been moved to its classes, however, this refactor broke some classes so we fixed all the test classes broke with issues related to Whitelist.
Motivation and Context
This relates to the Bridge Refactor to decouple some objects tied closely to the Bridge.
How Has This Been Tested?
Unit Tests.
Types of changes
Checklist: