LockingCap Refactor Integration #2672
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
marcos-iov
force-pushed
the
wip/locking-cap-refactor-integration
branch
2 times, most recently
from
aac73df to
be8b2ab
Compare
marcos-iov
force-pushed
the
federation-support-refactor-integration
branch
from
04e7eec to
2a832e7
Compare
marcos-iov
force-pushed
the
wip/locking-cap-refactor-integration
branch
from
be8b2ab to
2dd40f7
Compare
Base automatically changed from
federation-support-refactor-integration
to
master
August 9, 2024 15:49
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
rskj-core/src/test/java/co/rsk/peg/lockingcap/LockingCapStorageProviderImplTest.java
Fixed
Show fixed
Hide fixed
marcos-iov
force-pushed
the
wip/locking-cap-refactor-integration
branch
from
84a7135 to
cb9d7dc
Compare
Following with the Bridge refactors, it is now time to refactor and remove the locking cap business logic from the Bridge. Create its package lockingcap with a Support, StorageProvider, and Constants classes. Since it will have its classes should remove from the variable names, lockingCap, to avoid being redundant.
Create the package lockingcap with an enum to hold the storage index key for locking cap value. Create a new enum LockingCapStorageIndexKey to contain the index key for the locking cap value in storage, remove it from BridgeStorageIndexKey, remove the. _KEY suffix, and rename to LOCKING_CAP.
Create an interface LockingCapStorageProvider under co.rsk.peg.lockingcap package. Include the signature of the methods mentioned above. Rename saveLockingCap to simply save
Create a new interface LockingCapSupport under the co.rsk.peg.lockingcap package. Copy the signature and javadoc for the methods: getLockingCap and increaseLockingCap.
Following the Bridge refactors, we already migrated the LockingCapConstants to their classes to break the high coupling LockingCap had with the Bridge objects, however, it’s missing add coverage, so we need to create Unit Tests to get reliability that your new code doesn't break existing functionality when a change is made to the application. Create Unit Tests for all methods in LockingCapConstants Create Test Cases for all Networks such as MainNet, TestNet, and RegTest.
Adding An Optional To Return Value getLockingCap() Method
Removing Arrays.stream From increaseAuthorizedKeys Variable and replacing it with Collections.unmodifiableList(Stream.of()
Following with the Bridge refactors, it is now time to refactor and remove locking cap logic from BridgeSupport class. Create LockingCapConstants in BridgeConstants.
Following with the Bridge refactors, it is now time to refactor and remove locking cap logic from Bridge objects to LockingCap objects. This time we need to add Locking Cap logic to BridgeSupportBuilder class. Add WhitelistSupport to BridgeSupportBuilder business logic. Rebasing Changes With Integration Branch Rebasing Changes With Integration Branch
- To avoid an NoSuchElementException trying to get the value from Optional.get() when is Optional.empty(), use orElse(null) method. Assigning a value to an instance variable - Assigning a value to an instance variable instead of return and after that assign. Improving readiness, readability and maintainability - Instead of enclosing in an IF a block of code, we ensure we have the RSKIP134 to execute the rest of the code.
After LockingCapSupport interface has been created, it is time to implement it. Create a new class LockingCapSupportImpl under co.rsk.peg.lockingcap package that implements the methods defined in LockingCapSupport interface. Take the implementation from BridgeSupport class, and move to this new class. Adapting implementation to stop using Optional and activation as an input at a Support level Getting back Optional since there's a case when we need to handle a null value Improving indentation Avoiding Null Pointer Exception - If it doesn't exist Locking Cap value, it must return false since RSKIP134 is not active. Assigning a value from an Optional to a new variable Rollback the changes on BridgeSupport
Update BridgeSupportFactory to create an instance of LockingCapSupport and pass it to BridgeSupport constructor. Fix code smells
After LockingCapSupport interface has been created, we’re going to use it on BridgeSupport. Adapt LockingCap methods in BridgeSupport.Use new interface created, LockingCapSupport, under co.rsk.peg.lockingcap package to create LockingCap behavior. Rebase Code Smell Removing Optional from getLockingCap() Method at a LockingCapSupport Level
…ort level Getting back Optional since there's a case when we need to handle a null value and adapting verifyLockDoesNotSurpassLockingCap to use getLockingCap() from LockingCapSupport Removing IF statement related to activation and adding a new validation to improve Readiness and Maintainability
Following the Bridge refactors, we already migrated the LockingCap processes to their classes to break the high coupling LockingCap had with the Bridge objects. After refactoring, showed up different issues in the tests and it is time to fix them. Fix LockingCapTest class, and improve if possible.
Following the Bridge refactors, we already migrated the LockingCap processes to their classes to break the high coupling LockingCap had with the Bridge objects. After refactoring, showed up different issues in the tests and it is time to fix them. Fix BridgeSupportAddSignatureTest class.
Following the Bridge refactors, we already migrated the LockingCap processes to their classes to break the high coupling LockingCap had with the Bridge objects. After refactoring, showed up different issues in the tests and it is time to fix them. Fix BridgeSupportIT class.
Following the Bridge refactors, we already migrated the LockingCap processes to their classes to break the high coupling LockingCap had with the Bridge objects. After refactoring, showed up different issues in the tests and it is time to fix them. Fix BridgeSupportRegisterBtcTransactionTest class.
Following the Bridge refactors, we already migrated the LockingCap processes to their classes to break the high coupling LockingCap had with the Bridge objects. Please move the business logic related to the zero-negative LockingCap value from Bridge.java to LockingCapSupportImpl.java. This way, decouple the process and get all LockingCap business logic, in the LockingCapSupport object. Adding log for Zero - Negative LockingCap Value Refactoring increaseLockingCap tests in BridgeTest
- Using in LockingCap objects LockingCapIllegalArgumentException. - In Bridge objects VMException. Created INCREASE_LOCKING_CAP_LOG_TAG constant to avoid String repetition Bank line to improve readability When No Arguments Test Case, replaced encodeSignature with encodeArguments method Created a message string to avoid repetition Avoiding String Repetition Removed Unused Import Improving Log Message Improving Log Message for maxLockingCapVoteValueAllowed Case Improving Exception Handling for increaseLockingCap in Bridge objects Use ActivationConfigsForTest.all() instead of papyrus200() - Use ActivationConfigsForTest.all() instead of papyrus200() since it's not needed a specific fork in the most of the test cases Improving Logger Messages Improving Logger Messages
Get rid of getInstance in BridgeRegTestConstants Left the changes on LockingCapTest as it was early
Add coverage for getFeePerKbConstants, getWhitelistConstants, and getFederationConstants in BridgeConstants. Use wildcard imports Get rid of assertEquals in new Unit Tests
Following the Bridge refactors, we already migrated the LockingCapConstants to their classes to break the high coupling LockingCap had with the Bridge objects, however, it’s missing add coverage, so we need to create Unit Tests to get reliability that your new code doesn't break existing functionality when a change is made to the application. Create the Integration Test for the whole process related to LockingCap.
marcos-iov
force-pushed
the
wip/locking-cap-refactor-integration
branch
from
cb9d7dc to
1b9b7dc
Compare
marcos-iov
force-pushed
the
wip/locking-cap-refactor-integration
branch
from
8a81822 to
444d59a
Compare
marcos-iov
previously approved these changes
Aug 20, 2024
marcos-iov
force-pushed
the
wip/locking-cap-refactor-integration
branch
from
197b00b to
0d6dd00
Compare
marcos-iov
previously approved these changes
Aug 20, 2024
Remove unused import
marcos-iov
force-pushed
the
wip/locking-cap-refactor-integration
branch
from
0d6dd00 to
35e5a8d
Compare
|
marcos-iov
approved these changes
Aug 20, 2024
|
pipeline:run |
4 similar comments
|
pipeline:run |
|
pipeline:run |
|
pipeline:run |
|
pipeline:run |
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 refactors, now it is time to refactor and remove the
locking capbusiness logic from theBridge. In the same way, add coverage for the new objects created and make Unit Tests to get reliability so that your new code doesn't break existing functionality when a change is made to the application. Below the changes included:lockingcap, with aSupport,StorageProvider, andConstantsclasses. Since it will have its classes should be removed from the variable names, lockingCap, to avoid being redundant.LockingCapConstantsinBridgeConstants.LockingCapStorageIndexKeyto contain the index key for the locking cap value in storage, remove it fromBridgeStorageIndexKey, remove the_KEY suffix, and rename it toLOCKING_CAP.LockingCapStorageProviderunderco.rsk.peg.lockingcappackage. Include the signature of the methods mentioned above. RenamesaveLockingCapto simply save.LockingCapStorageProviderImpl.LockingCapSupportImpl.LockingCapSupportunder theco.rsk.peg.lockingcappackage. Copy the signature and javadoc for the methods:getLockingCapandincreaseLockingCap.LockingCapSupportinterface has been created, it is time to implement it. Create a new classLockingCapSupportImplunderco.rsk.peg.lockingcappackage that implements the methods defined in theLockingCapSupportinterface. Take the implementation from theBridgeSupportclass, and move to this new class.maxLockingCapVoteValueAllowedinLockingCapSupportImplLockingCapConstants, and create Test Cases for all Networks such as MainNet, TestNet, and RegTest.WhitelistSupportto theBridgeSupportBuilderbusiness logic.BridgeSupport.LockingCapvalue fromBridge.javatoLockingCapSupportImpl.java.LockingCapIllegalArgumentExceptionincreaseLockingCapinBridgeobjectsMotivation 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: