Router Filter Modularisation and Test suite #5177
Conversation
Jenkins BuildsClick to see older builds (72)
|
|
|
|
I thought of a few more conditions for
|
|
Interesting fails on test runs. See here for more details https://github.com/status-im/samyoul-notes/blob/master/attachments/2024-05/filter_test_results.md |
|
State of tests: 1.
|
|
Ok it seems that many of these fails are a result of overly precise comparisons expecting even the memory addresses to be identical. I'll tweak those tests to compare values rather than expecting 100% IDENTICAL values. |
saledjenic
force-pushed
the
router-improvements-general
branch
from
5deb408
to
f7bc3d4
Compare
|
@Samyoul could you rebase it, please? |
There was a problem hiding this comment.
@Samyoul nice modularisation, thanks, I left a few questions.
| } | ||
|
|
||
| // setupRouteValidationMapsV2 initializes maps for network inclusion and exclusion based on locked amounts. | ||
| func setupRouteValidationMapsV2(fromLockedAmount map[uint64]*hexutil.Big) (map[uint64]bool, map[uint64]bool) { |
There was a problem hiding this comment.
Maybe we should check the validity of fromLockedAmount at the moment when SuggestedRoutesV2 is called.
We should check that are all entries are positive, I guess that if the user deliberately doesn't want to send any amount from a certain chain there are two options to do that from the UI side:
- disable that chain (which means we should add that chain to
DisabledFromChainIDsarray) - or set 0 value for locked amount on that chain
There was a problem hiding this comment.
That's a good idea. We need some better validation generally.
There was a problem hiding this comment.
@saledjenic I've created an issue for this #5227
services/wallet/router/filter.go
Outdated
| fromExcluded := make(map[uint64]bool) | ||
|
|
||
| for chainID, amount := range fromLockedAmount { | ||
| if amount.ToInt().Cmp(zero) == 0 { |
There was a problem hiding this comment.
Until we add the check for fromLockedAmount validation, we should check if amount.ToInt().Cmp(zero) <= 0 {, right?
services/wallet/router/filter.go
Outdated
| for _, path := range route { | ||
| if amount, ok := fromLockedAmount[path.From.ChainID]; ok { | ||
| requiredAmountIn := new(big.Int).Sub(amountIn, amount.ToInt()) | ||
| if totalRestAmount.Cmp(requiredAmountIn) < 0 { |
There was a problem hiding this comment.
Doesn't the logic need to be like this?
if amount, ok := fromLockedAmount[path.From.ChainID]; ok {
requiredAmountIn := new(big.Int).Sub(amountIn, amount.ToInt())
totalRestAmount.Sub(amountIn, amount.ToInt()) // <===
if totalRestAmount.Cmp(requiredAmountIn) <= 0 {
return false
}
path.AmountIn = amount
path.AmountInLocked = true
totalRestAmount.Sub(totalRestAmount, amount.ToInt())
}
|
Hey @saledjenic thank you for your review. I definitely need to review the logic of the modular functions. I was just trying to build out the modular functionality and then add the tests to cover all the test cases I could think of. I think I may have spotted some cases that have revealed some areas of concern, but after I've checked the logic I can confirm this. I'll ping you when I'm ready for you to look over this PR again. |
|
Hmmm, having some rebase issues:
|
|
Oh boy I've got some serious problems: |
saledjenic
force-pushed
the
router-filter-modularisation
branch
from
b72ea16
to
ab78ecb
Compare
|
Ok I've got another issue: So git says that Also with |
|
Let's try status-go % git restore --staged shell.nix
status-go % git status
On branch router-filter-modularisation
Changes to be committed:
(use "git restore --staged <file>..." to unstage)
deleted: shell.nixOk, what about status-go % git add shell.nix
status-go % git status
On branch router-filter-modularisation
Changes to be committed:
(use "git restore --staged <file>..." to unstage)
deleted: shell.nixOk, let's try actually deleting it: status-go % rm shell.nix
status-go % cat shell.nix
cat: shell.nix: No such file or directoryLet's commit that change: status-go % git commit -S -a -m "Deleting shell.nix for some reason"
[router-filter-modularisation f7bb4ef6f] Deleting shell.nix for some reason
2 files changed, 109 deletions(-)
delete mode 100644 shell.nix
delete mode 100644 telemetry/client.go
status-go % git status
On branch router-filter-modularisation
nothing to commit, working tree clean
Reset the branch to the previous commit: status-go % git reset b72ea163880fdf90349320d8e02429ddf18a4e69 --hard
HEAD is now at b72ea1638 Added much better coverage for testing for filterNetworkComplianceV2Now checking it is all back to normal. 😱 !!! |
|
My IDE's GUI git tool also reports the file as missing while at the same time showing that the file is present. 
One thing that I notice is that the file name is grey, meaning that the file is being # Nix
/.nix-gcroots/Is some local setting ignoring it? |
Samyoul
force-pushed
the
router-filter-modularisation
branch
from
ab78ecb
to
f6c6a86
Compare
saledjenic
force-pushed
the
router-filter-modularisation
branch
from
f6c6a86
to
ab78ecb
Compare
Samyoul
force-pushed
the
router-filter-modularisation
branch
from
ab78ecb
to
2c8af7c
Compare
Samyoul
changed the title
|
Ugh!
Looks like I need to rename all 17 of my commits so far. |
|
Danger zone - addressing concerns Danger 1:
As you already said, this issues will fix that #5244 Danger 2:
For this one, the input parameters are wrong as I see.
Now having all that in mind and params the test case you've provided we see that the only route in routes contains a single path which is Danger 3:
Issues in this case are pretty similar as described in "Danger 2", so if you have this @Samyoul I hope that helps in understanding what we want. |
Ok thank you, @saledjenic, so in this case the test case is correct, this scenario is impossible.
I think that for this case we need to introduce additional validation to ensure that routes only contain a single instance of a network. Although this test fails, it would pass with other parameters. EDIT: I've updated #5227 to include the need to ensure routes do not include multiple instances of the same network. |
There was a problem hiding this comment.
@Samyoul comments here are getting bigger and bigger and harder to follow, I guess that you mapped the old logic we had to the new form, you've splited the logic very well that it can be maintained easear now, so please, let's merge it if it doesn't break anything we had and continue improving it through the issues you've created?
Samyoul
force-pushed
the
router-filter-modularisation
branch
from
2a7f667
to
3518511
Compare
This is as per my original draft earlier this week https://github.com/status-im/samyoul-notes/blob/master/analysis/wallet/Router/code/filterRoutes.go
…anceV2 Note there are a number of failing test, this is fine. Need to figure out if these are caused by my tests, my code or the original algorithm
Samyoul
force-pushed
the
router-filter-modularisation
branch
from
3518511
to
e01dab6
Compare
|
All commits are now reworded to prepend |
Building on the work of #5159, this PR breaks the
filterRoutesV2function down into its main components. This increases the testability and ability to identify places where the code may break in the future.My original draft is here See here for details
Summary of Each Function
filterRoutesV2Filters routes based on network compliance and capacity validation. It first checks if the
fromLockedAmountmap is empty, returning the routes unmodified if so. Otherwise, it applies network compliance filtering followed by capacity validation filtering.filterNetworkComplianceV2Performs the first level of filtering based on network inclusion and exclusion criteria. It initializes inclusion and exclusion maps based on the
fromLockedAmountmap and then iterates through each route to ensure compliance with these criteria. Only routes that pass the compliance check are included in the filtered routes.isValidForNetworkComplianceV2Checks if a given route complies with network inclusion and exclusion criteria. It logs the initial inclusion and exclusion maps, then iterates through each path in the route. If any path's
ChainIDis in the exclusion map or if any required inclusionChainIDis missing, the route is deemed non-compliant and returns false. Otherwise, it returns true.setupRouteValidationMapsV2Initializes maps for network inclusion and exclusion based on the
fromLockedAmountmap. Chains with zero or negative amounts are marked as excluded, while chains with positive amounts are marked as included.filterCapacityValidationV2Performs the second level of filtering based on amount and capacity validation. It iterates through the routes and checks if each route has sufficient capacity to handle the required amount using the
hasSufficientCapacityV2function. Only routes that have sufficient capacity are included in the filtered routes.hasSufficientCapacityV2Checks if a route has sufficient capacity to handle the required amount. For each path in the route, it calculates the required amount by subtracting the path's locked amount from the input amount. It then calculates the remaining amount in the route excluding the current path. If the remaining amount is greater than or equal to the required amount, the path is marked as having sufficient capacity. Otherwise, the route is deemed insufficient and returns false.
calculateRestAmountInV2Calculates the remaining amount in for the route, excluding a specified path. It iterates through the paths in the route, summing up the amounts for all paths except the excluded one, and returns the total remaining amount.
Summary of Test Cases for each tested function
TestSetupRouteValidationMapsV2This test checks the function
setupRouteValidationMapsV2which initializes maps for network inclusion and exclusion based on locked amounts.Test Cases:
TestCalculateRestAmountInV2This test verifies the function
calculateRestAmountInV2which calculates the remaining amount in for the route excluding the specified path.Test Cases:
TestIsValidForNetworkComplianceV2This test validates the function
isValidForNetworkComplianceV2which checks if a route complies with network inclusion/exclusion criteria.Test Cases:
TestHasSufficientCapacityV2This test validates the function
hasSufficientCapacityV2which checks if a route has sufficient capacity to handle the required amount.Test Cases:
TestFilterNetworkComplianceV2This test validates the function
filterNetworkComplianceV2which performs the first level of filtering based on network inclusion/exclusion criteria.Test Cases:
TestFilterCapacityValidationV2This test checks the function
filterCapacityValidationV2which performs the second level of filtering based on amount and capacity validation.Test Cases:
TestFilterRoutesV2This test validates the overall
filterRoutesV2function which filters routes based on network compliance and capacity validation.Test Cases:
🚨 Danger Zone! 🚨
There are 3 test cases in this PR that I have questions about:
When testing
hasSufficientCapacityV2why does the current test case returntruewhen it should befalse?When testing
filterCapacityValidationV2with a single route with sufficient capacity, I expect this test to return a route composed of{From: network1, AmountIn: (*hexutil.Big)(big.NewInt(200))}, but the logic gives an empty route.This seems incorrect to me. But why?
{ // TODO Is the behaviour of this test correct? It looks wrong name: "Single route with sufficient capacity", routes: [][]*PathV2{ { {From: network1, AmountIn: (*hexutil.Big)(big.NewInt(200))}, }, }, amountIn: big.NewInt(150), fromLockedAmount: map[uint64]*hexutil.Big{ 1: (*hexutil.Big)(big.NewInt(50)), }, expectedRoutes: [][]*PathV2{}, },When testing
filterCapacityValidationV2we should probably receive an error, but also in this case the criteria seems to be met. But the test case expects an empty route... This seems wrong to me.{ // TODO this seems wrong also. Should this test case work? name: "Routes with duplicate chain IDs", routes: [][]*PathV2{ { {From: network1, AmountIn: (*hexutil.Big)(big.NewInt(100))}, {From: network1, AmountIn: (*hexutil.Big)(big.NewInt(100))}, }, }, amountIn: big.NewInt(150), fromLockedAmount: map[uint64]*hexutil.Big{ 1: (*hexutil.Big)(big.NewInt(50)), }, expectedRoutes: [][]*PathV2{}, },