test_: added tests for updating fees when after router provides the best route #5795
Conversation
saledjenic
requested review from
alaibe,
shivekkhurana,
antdanchenko and
alexjba
|
Thank you for opening this pull request! Looks like you have BREAKING CHANGES in your PR. |
Jenkins BuildsClick to see older builds (99)
|
|
|
||
| closeFn := func() { | ||
| close(suggestedRoutesCh) | ||
| signal.SetMobileSignalHandler(nil) |
There was a problem hiding this comment.
Thanks for keeping an eye on the reset of MobileSignalHandler ❤️
I recently added a ResetMobileSignalHandler, please checkout this
TLDR: Just add t.Cleanup(signal.ResetMobileSignalHandler) instead, it's a bit simpler.
| var allGeneratedPaths []string | ||
| allGeneratedPaths = append(allGeneratedPaths, pathEIP1581Root, pathEIP1581Chat, pathWalletRoot, pathDefaultWalletAccount) |
There was a problem hiding this comment.
🤔
| var allGeneratedPaths []string | |
| allGeneratedPaths = append(allGeneratedPaths, pathEIP1581Root, pathEIP1581Chat, pathWalletRoot, pathDefaultWalletAccount) | |
| allGeneratedPaths := []string{pathEIP1581Root, pathEIP1581Chat, pathWalletRoot, pathDefaultWalletAccount} |
| err = backend.StartNodeWithAccountAndInitialConfig(account, password, *defaultSettings, nodeConfig, profileKeypair.Accounts, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| return backend, stop, nil |
There was a problem hiding this comment.
Same here for stop, using t.Cleanup help
saledjenic
force-pushed
the
router-updates-tests-1
branch
2 times, most recently
from
552b757
to
d52622f
Compare
|
@igor-sirotin thanks for the review, and comments addressed. |
|
Tests seem to be broken, investigating |
There was a problem hiding this comment.
Don't take this personally, just to ensure it's not merged while the tests are failing 😄
Details: #5797
Yes, I know that. Discussed with Anton, already. :) |
|
Accidents happen, you know 😄 |
saledjenic
force-pushed
the
router-clean-up
branch
from
f655fa5
to
7e6d19f
Compare
saledjenic
force-pushed
the
router-updates-tests-1
branch
from
257a72b
to
0a4156b
Compare
saledjenic
force-pushed
the
router-clean-up
branch
from
7e6d19f
to
1242ab7
Compare
saledjenic
force-pushed
the
router-updates-tests-1
branch
2 times, most recently
from
f785e06
to
6129a75
Compare
|
@igor-sirotin @antdanchenko please take a look at the tests now. |
saledjenic
force-pushed
the
router-updates-tests-1
branch
from
6129a75
to
a0869cc
Compare
…as router v2 Breaking change! Since the old router logic is removed, there is no need to have files and structs marked as v2. Renamed endpoints: - `GetSuggestedRoutesV2` to `GetSuggestedRoutes` - `GetSuggestedRoutesV2Async` to `GetSuggestedRoutesAsync` - `StopSuggestedRoutesV2AsyncCalcualtion` to `StopSuggestedRoutesAsyncCalcualtion`
It's a breaking change due to errors' changes, the list of mapped old error codes: - `"WR-001"` is now `"WRR-001"` - `"WR-002"` is now `"WRR-002"` - `"WR-003"` is now `"WRR-003"` - `"WR-004"` is now `"WRR-004"` - `"WR-005"` is now `"WRR-005"` - `"WR-006"` is now `"WRR-006"` - `"WR-007"` is now `"WRR-007"` - `"WR-008"` is now `"WRR-008"` - `"WR-009"` is now `"WRR-009"` - `"WR-010"` is now `"WRR-010"` - `"WR-011"` is now `"WRR-011"` - `"WR-012"` is now `"WRR-012"` - `"WR-013"` is now `"WRR-013"` - `"WR-014"` is now `"WRR-014"` - `"WR-015"` is now `"WRR-015"` - `"WR-019"` is now `"WRR-016"` - `"WR-020"` is now `"WRR-017"` - `"WR-021"` is now `"WRR-018"` - `"WR-025"` is now `"WRR-019"` - `"WR-016"` is now `"WR-001"` - `"WR-017"` is now `"WR-002"` - `"WR-018"` is now `"WR-003"` - `"WR-022"` is now `"WR-004"` - `"WR-023"` is now `"WR-005"` - `"WR-024"` is now `"WR-006"` - `"WR-026"` is now `"WR-007"` - `"WR-027"` is now `"WR-008"` Other changes: - `RouteInputParams` type moved to `requests` package and code updated accordingly - `SuggestedFees` type moved to a new `fees` package and code updated accordingly - `SendType` type moved to a new `sendtype` package and code updated accordingly - the following functions moved to `common` package - `ArrayContainsElement` - `ArraysWithSameElements` - `SameSingleChainTransfer` - `CopyMapGeneric` - `GweiToEth` - `WeiToGwei` - the following consts moved to `common` package - `HexAddressLength` - `SupportedNetworks` - `SupportedTestNetworks`
Following the approach we did for keeping requests at the same location, these changes introduce new location for responses. `SuggestedRoutesResponse` is moved there and renamed to `RouterSuggestedRoutes` and code is updated accordingly. New type `Route` defined (since a single route is composed of zero or more paths). Types `Route`, `Path`, `Graph` and `Node` belong to a new `routs` package now.
saledjenic
force-pushed
the
router-clean-up
branch
from
1242ab7
to
557c4f7
Compare
saledjenic
force-pushed
the
router-updates-tests-1
branch
from
a0869cc
to
7b1795c
Compare
saledjenic
force-pushed
the
router-updates-tests-1
branch
from
9b09420
to
a359cd6
Compare
saledjenic
force-pushed
the
router-updates-tests-1
branch
from
a359cd6
to
0206dff
Compare
| @@ -3,7 +3,7 @@ services: | |||
| image: ghcr.io/foundry-rs/foundry:latest | |||
| platform: linux/amd64 | |||
| command: | |||
| - anvil --host 0.0.0.0 | |||
| - anvil --host 0.0.0.0 --block-time 2 | |||
There was a problem hiding this comment.
we don't need this change for integration-tests
| - ../:/go/src/github.com/status-im/status-go/ | ||
| working_dir: /go/src/github.com/status-im/status-go/api | ||
| command: | | ||
| go test -tags gowaku_skip_migrations -run ^TestRouterFeesUpdate github.com/status-im/status-go/api/... -v -- -anvil-host=http://anvil:8545 |
There was a problem hiding this comment.
right now we are launching a single test here, would be great to replace it with launching all go tests that require anvil if possible
saledjenic
force-pushed
the
router-updates-tests-1
branch
from
0206dff
to
d0e6e66
Compare
| @@ -499,3 +499,6 @@ run-integration-tests: | |||
| run-anvil: SHELL := /bin/sh | |||
| run-anvil: | |||
| docker-compose -f integration-tests/docker-compose.anvil.yml up --remove-orphans | |||
|
|
|||
| run-integration-anvil: | |||
There was a problem hiding this comment.
CI run won't finish because anvil will continue to run forever, we need to do something similar to https://github.com/status-im/status-go/blob/develop/Makefile#L492 with running silently and then reading exit code from the container with tests
saledjenic
force-pushed
the
router-updates-tests-1
branch
3 times, most recently
from
29ed647
to
2ae40f8
Compare
saledjenic
force-pushed
the
router-updates-tests-1
branch
from
b0ecc04
to
a34efcd
Compare
|
I probably should have looked at this closer in the very beginning, but better to speak late than never 🙂
cc @antdanchenko @saledjenic @status-im/status-go-guild These are some major questions I have. |
Exactly, that's what I said to you in private chat, we have to sort that out a bit better cause
Yes, looks like that, but we need
I am not sure that we can, cause those The point of having them this way is that we will add more tests later here that will test the router, sending transaction process and that way we can get rid of I agree that we should think about this, so no rush in making a decision. This PR can stay like this, at least as a PoC, till we figure out what's the best approach to proceed with. @igor-sirotin thanks for the review, there is a long way in front of us till we set everything properly. |
saledjenic
force-pushed
the
router-updates-tests-1
branch
from
a34efcd
to
0867efd
Compare
saledjenic
force-pushed
the
router-updates-tests-1
branch
from
0867efd
to
4d1a48f
Compare
|
We agreed to postpone this PR. The main reason why we can't implement this with existing integration tests, is the disability of getting events from status-go. I opened an issue about it here: It shouldn't be difficult to implement, I will take a look at this ASAP. |
|
Here's the PR with required functionality implemented: |
saledjenic
force-pushed
the
router-clean-up
branch
2 times, most recently
from
cbb74b7
to
900ad9b
Compare
This PR introduces anvil network on the status-go side and tests: