Router clean up #5778
Router clean up #5778
Conversation
|
Thank you for opening this pull request! Looks like you have BREAKING CHANGES in your PR. |
Jenkins BuildsClick to see older builds (61)
|
saledjenic
requested review from
ilmotta,
alaibe,
shivekkhurana,
dlipicar and
igor-sirotin
services/wallet/common/utils.go
Outdated
| return false | ||
| } | ||
| for _, el := range ar1 { | ||
| if !ArrayContainsElement(el, ar2) { |
There was a problem hiding this comment.
What do you mean isEqual here? I am checking if an element el of type T is in an array of elements []T.
There was a problem hiding this comment.
you've got an argument isEqual func(T, T) bool in function ArraysWithSameElements which is not getting used. I'm assuming you want that for cases where you have arrays of pointers, to do a "deep" comparison instead of just comparing the pointers themselves.
Maybe the same can be achieved using https://pkg.go.dev/reflect#DeepEqual in ArrayContainsElement instead of ==
There was a problem hiding this comment.
Well actually I think the whole function is unused after the rebase, you can remove ArraysWithSameElements
There was a problem hiding this comment.
Ahh I see what you mean, so my intention was to provide isEqual to ArraysWithSameElements and then pass it to ArrayContainsElement to compare any type using a custom isEqual function, but later when I removed it from ArrayContainsElement, forgot to remove it from ArraysWithSameElements.
But yes, it's not in use anymore, removed now.
| @@ -0,0 +1,67 @@ | |||
| package routs | |||
There was a problem hiding this comment.
did you mean routes here?
There was a problem hiding this comment.
Exactly that! :)
|
Thumbs up for better code organization into packages and old code removal. |
Was thinking about that, but two reasons made me make this change:
but then decided for this, cleaner way, since it's not a big deal at this moment. |
saledjenic
force-pushed
the
router-clean-up
branch
from
e57bf22 to
e8fafb0
Compare
There was a problem hiding this comment.
I don't understand why is this needed.
- When we're release WakuV2, we don't forget WakuV1 and rename WakuV2 to WakuV1.
- What if we release a newer version of
router? Will it beRouterV2orRouterV3then?
I believe this breaking change is very unnecessary.
Same about renaming files. Why?
There was a problem hiding this comment.
Router V2 was just a replacement for the old router, to not mess up with the code, while we're improving it. Also I don't like V2 in endpoint names, if we really want to have versioning then it should be done another way and passed as a property of input params.
There was a problem hiding this comment.
if we really want to have versioning then it should be done another way and passed as a property of input params.
Or even have a separate service for this 🤔
Not a big deal. If the desktop/mobile will be adapted then it's 90% ok IMO.
Just in general, it's important to have a mindset of not introducing any breaking changes, I guess that's my point
| @@ -488,31 +488,6 @@ func gweiToWei(val *big.Float) *big.Int { | |||
| return res | |||
| } | |||
|
|
|||
| func (api *API) GetSuggestedRoutes( | |||
There was a problem hiding this comment.
Just mark it as deprecated and return an error for now. And remove the function when the clients are updated.
No need for such destructive actions 😄
There was a problem hiding this comment.
But I wanted to clean the api, not to keep things around, that's why we added the same names with V2 suffixes, to not make breaking changes. Also I plan to sync with @alwx (actually we already chat in the wallet channel) on this and don't plan to merge this before mobile is ready for this change, then we will sync and merge changes at the same time.
saledjenic
force-pushed
the
router-clean-up
branch
from
e8fafb0 to
d31695a
Compare
| @@ -131,7 +111,11 @@ func (s SendType) canUseProcessor(p pathprocessor.PathProcessor) bool { | |||
| } | |||
| } | |||
|
|
|||
| func (s SendType) processZeroAmountInProcessor(amountIn *big.Int, amountOut *big.Int, processorName string) bool { | |||
| func (s SendType) SimpleTransfer(p pathprocessor.PathProcessor) bool { | |||
There was a problem hiding this comment.
I think this is unused now. Moreover, we normally call these something like isSimpleTransfer
There was a problem hiding this comment.
@igor-sirotin I thought the linter checked unused functions and function parameters 🤔 did we disable that or did I just make that up?
There was a problem hiding this comment.
Correct, after rebase, not needed.
There was a problem hiding this comment.
@igor-sirotin I thought the linter checked unused functions and function parameters 🤔 did we disable that or did I just make that up?
Indeed I think it was there. We need to review linter checks and ensure it's there 👍
saledjenic
force-pushed
the
router-clean-up
branch
from
d31695a to
2abacca
Compare
saledjenic
force-pushed
the
router-clean-up
branch
from
2abacca to
f870d5c
Compare
|
The desktop PR that handles these changes is here: |
saledjenic
force-pushed
the
router-clean-up
branch
from
5704939 to
0e20c00
Compare
|
FYI @shivekkhurana there are breaking changes as described in the PR and we should prepare mobile @saledjenic @shivekkhurana for a larger PR like this one, before merging, it's paramount to get the PR tested on the mobile client too by our QAs. The idea being to keep status-go |
|
@ilmotta yes, sure, mobile QAs can start testing after @alwx integrates these changes into the mobile app. Just for context... yes, some commits are marked as breaking changes, but that's due to the API endpoints name change, so changes should not affect anything especially (referring to clean-up changes, the other two commits are from other PR, which is about updating fees, but they should break anything) |
saledjenic
force-pushed
the
router-clean-up
branch
5 times, most recently
from
557c4f7 to
cbb74b7
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
cbb74b7 to
900ad9b
Compare
Hi @saledjenic, I came across this PR again today and I would like to take this opportunity to raise some concerns and ideas for future PRs. We should generally avoid large PRs like this one, especially in status-go (but not exclusively). It's tempting to create large PRs to avoid the overhead of reviews + QA, but large PRs can be counterproductive. A few reasons:
Probably a few other reasons I couldn't think of now. One of our main goals should be to ensure that status-go's There's almost always a way to make incremental changes that simplify testing and can result in a higher-quality results in the product. This PR could have been split into smaller ones. Perhaps the @status-im/status-go-guild will be able to define more clear guidelines about the points I'm raising here? On a more personal level, in the status-mobile repo I have a tendency to immediately reject very large PRs (they are rare thankfully) and ask the author to make safer incremental changes in separate PRs. We all opened our fair share of large PRs in life 🥶, but in status-go and with the ever increasing complexity of the repo I think we should plainly avoid them. |
|
@ilmotta I understand what you're saying, but there are 2 PRs:
The important thing is that both PRs were reviewed separately and then the second was merged into the first one. So basically those were 2 simple PRs for review. Also since it's about removing unused code and renaming only, I don't expect any incompatibilities on the client sides (at least there were none on the desktop app side). Changes in this PR are marked as breaking changes, just because of endpoints renaming. |
Thanks for sharing the specifics @saledjenic. I'm aware of some of these details. My comment is about the perspective that large PRs cause more trouble than benefit, so should be avoided if there are potentially more downsides than upsides. I'm not sharing these concerns only for this PR. In other words, is the increased risk of regressions, slower turnaround to merge, increased time requested from reviewers & QAs, and (likely) reduced review quality worth the trouble? Smaller PRs are helpful to diagnose where regressions come from. Many times I had to checkout commits in status-go to identify where a bug was first introduced. Because we squash when merging, large squashed commits become quite difficult to comprehend. Just an example for this PR: renames and file moves could've been done in a separate PR from feat commits and breaking changes. |
Hi @ilmotta, I appreciate your perspective, but I see things a bit differently. In my opinion, it's perfectly fine to have logically split commits within a single PR, but creating a new PR for every logical unit might become a bit cumbersome and burdensome at the same time. Of course, I'm open to further discussion.
Yes, I agree it's hard to find the culprit for some regression looking at a large commit if all commits were squashed at the moment of merging into the develop branch, but I didn't know that's what people do?!? All my PRs (branches) are rebased on the develop branch with commits like people see them in the PR. Also when one is searching for a regression, he is checking a certain commit and doesn't care if it's coming from a PR with a single commit only or PR that has 10 commits. That's why it's important to have every single commit for a single logical change and see them in the develop branch as they are in PRs. |
|
@ilmotta Having multiple working commits seems perfectly fine to me. I'd say it even simplify things. Now most PR will have 1 commit, but if there is more and the PR is still consistent, why not? Is there a specific concern with that approach? I understand that additional commits were added because the first one contained breaking changes. It's natural to avoid working on outdated code, so I think the question worth asking is what might be causing the renaming of three functions to take longer than expected (two weeks so far). Go can be verbose, and as a result, code changes may appear larger. However, the real factor to consider is complexity, not just size. Initially, the task was to remove old code |
|
Confirmed here: |
@alaibe As a starting point for the discussion I think the PR is the best place because it is a live example of a problem. Now that there's some disagreement on the best approach yes, it seems natural to move outside the PR the discussion.
I agree with you all on that @saledjenic @alaibe, but that's not what I said. The problem I raised is about large PRs and that they can create more downsides than upsides. I ask you to reconsider the problem from the general perspective of keeping status-go releasable and reduce the risks of regressions per unit of PR because this is a serious problem in status-go. Short-lived branches sliced in a smart way can help a lot. At no point I said one commit per PR is good, that'd be absurd 👍🏼
This point I don't fully understand. Do you mean it took too long to rename on the client? The cause is probably of mismatched expectations between the PR author and the client developers who would apply the patch. They didn't agree about the urgency, and naturally adjusting client code to refactors in status-go can easily go lower in priority against bugs. So both parties should talk more/better.
Precisely, removing old code can be done in a separate PR, very low risk, much quicker to review and merge if not mixed in with other changes that require manual QA. |
Reviewing this PR is much easier if commit by commit is reviewed since all changes are logically split into 4 commits.
Within the clean-up and files/code reorganizational job, the following changes were conducted:
old router logic removed
Old code removed, necessary from the old file functions moved to new files.
removed v2 mark from api endpoints, structs and files marked as router v2
Breaking change!
Since the old router logic has been removed, there is no need to have files and structures marked as v2.
Renamed endpoints:
GetSuggestedRoutesV2toGetSuggestedRoutesGetSuggestedRoutesV2AsynctoGetSuggestedRoutesAsyncStopSuggestedRoutesV2AsyncCalcualtiontoStopSuggestedRoutesAsyncCalculationrouter code organization improvements
Breaking change!
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:
RouteInputParamstype moved torequestspackage and code updated accordinglySuggestedFeestype moved to a newfeespackage and code updated accordinglySendTypetype moved to a newsendtypepackage and code updated accordinglycommonpackageArrayContainsElementArraysWithSameElementsSameSingleChainTransferCopyMapGenericGweiToEthWeiToGweicommonpackageHexAddressLengthSupportedNetworksSupportedTestNetworksrouter response moved to wallet responses location
Following our approach for keeping requests at the same location, these changes introduce new location for responses.
SuggestedRoutesResponseis moved there and renamed toRouterSuggestedRoutesand the code is updated accordingly.A new type
Routeis defined (since a single route is composed of zero or more paths).Types
Route,Path,GraphandNodebelong to a newroutespackage now.