Feat show all network and assets portfolio #17796
Conversation
Pavneet-Sing
added
CI/skip
android/java/org/chromium/chrome/browser/crypto_wallet/activities/AssetDetailActivity.java
Outdated
Show resolved
Hide resolved
android/java/org/chromium/chrome/browser/crypto_wallet/activities/AssetDetailActivity.java
Outdated
Show resolved
Hide resolved
android/java/org/chromium/chrome/browser/crypto_wallet/activities/AssetDetailActivity.java
Outdated
Show resolved
Hide resolved
android/java/org/chromium/chrome/browser/crypto_wallet/activities/AssetDetailActivity.java
Show resolved
Hide resolved
android/java/org/chromium/chrome/browser/crypto_wallet/activities/BraveWalletBaseActivity.java
Show resolved
Hide resolved
Pavneet-Sing
force-pushed
the
feat-show-all-network-and-assets-portfolio
branch
from
0361c3c
to
d5b3c0c
Compare
| * View <=> NetworkSelection state only with All Networks option. | ||
| * @param key as identifier to bind local state of NetworkSelection with the view. If null then | ||
| * use global/default network selection mode. | ||
| ^ IMP: Should only be called if the wallet is set up and unlocked |
There was a problem hiding this comment.
❤️ Excellent Javadoc!
I would probably remove the ^ character at the beginning of the line, just to honor the Javadoc syntax, but it's not a big deal.
To give relevance to the last sentence, a possible opening could be <b>Note:</b>.
| ^ IMP: Should only be called if the wallet is set up and unlocked | |
| * <b>Note:</b>: It should only be called if the wallet is set up and unlocked. |
| @@ -52,8 +58,11 @@ public class PortfolioModel implements BraveWalletServiceObserverImplDelegate { | |||
| private BraveWalletService mBraveWalletService; | |||
| private AssetRatioService mAssetRatioService; | |||
| private Context mContext; | |||
There was a problem hiding this comment.
Question unrelated to the scope of this PR: is it safe to save context in the models? Should we strive for models that are context free?
We are not handling rotations now (YAY), but the risk of memory leaks could still bite us in some rare edge cases.
There was a problem hiding this comment.
I haven't seen any issue with this though discuss with @SergeyZhukovsky to check GPS crash logs.
Feel free to create an issue for edge cases if you come across any or can reproduce any, then we can take it from there.
There was a problem hiding this comment.
yeah, I haven't seen any issues related to that. It's a valid point though, memory leaks exist, we have plenty of crashes related to not enough memory, it's unknown is it Brave that causes them or a device in general.
There was a problem hiding this comment.
Created a new GitHub issue to keep track of it: brave/brave-browser#29410
Feel free to modify or improve the description!
| mJsonRpcService, mSharedData.getSupportedCryptoCoins(), allNetworks -> { | ||
| mAllNetworkInfos = new ArrayList<>(allNetworks); | ||
| if (selectedNetwork.chainId.equals( | ||
| NetworkUtils.getAllNetworkOption(mContext).chainId)) { |
There was a problem hiding this comment.
Nothing to change here, but I was just wondering if we really need a context as braveWalletBaseActivity should probably suffice.
This method is the only place where mContext is used.
Not something to address in this PR, so please don't consider this as a change request.
There was a problem hiding this comment.
It doesn't matter here as both can be used and it won't make any difference in either cases so nothing to change.
| private void fetchUserAssetsAndDetails( | ||
| PortfolioHelper portfolioHelper, Callbacks.Callback1<PortfolioHelper> callback) { | ||
| portfolioHelper.fetchAllAssetsAndDetails(callback); | ||
| } | ||
|
|
There was a problem hiding this comment.
Considering this method is just one single line, I would remove this method entirely and simply replace its occurrences with portfolioHelper.fetchAllAssetsAndDetails(callback);.
There was a problem hiding this comment.
Don't think it is required, no harm in using same method at two places, even for small things.
I would strongly suggest to add nit for subjective comments to focus on primary concerns first and to avoid blocking PR merging.
There was a problem hiding this comment.
I would strongly suggest to add nit for subjective comments
I do add nit when it's the right case.
I'm considering this method as something that does not add any tangible value I'm afraid :-(
It's also considered an antipattern (AHA), it hides the real implementation, it does not avoid any code duplication as it's a single line, and even the name does not add any benefits as fetchUserAssetsAndDetails calls PortfolioHelper#fetchAllAssetsAndDetails.
Just to be very transparent: I'm not against one-liner methods. it's okay to use them when they add value. But here it's just a proxy method calling a method of PortfolioHelper with the exact same name.
void doSomething() {
portfolioHelper.doSomething();
}Removing this method makes the code easier to follow, easier to maintain, and the entire class slightly smaller. I don't see any valid reason that should lead us to accept such an antipattern.
Further links to delve deeper into the topic:
There was a problem hiding this comment.
I think we can both find pros and cons of each side. The idea here is to construct the portfolioHelper and perform the fetching in a separate method.
Won't consider this as an ideal candidate of AHA or one-liner because the idea here is to separate construction and calling. Additionally, it has been simplified to use the same method instead of calling fetchUserAssetsAndDetails and calculateBalances for single and multiple network value.
Here's some more details on Nits: https://google.github.io/eng-practices/review/reviewer/standard.html#:~:text=Reviewers%20should%20always%20feel%20free,they%20could%20choose%20to%20ignore.
Reviewers should always feel free to leave comments expressing that something could be better, but if it’s not very important, prefix it with something like “Nit: “ to let the author know that it’s just a point of polish that they could choose to ignore.
Again, many things can be subjective here.
There was a problem hiding this comment.
@Pavneet-Sing I also find that method confusing. Our code base is going to blow If we continue that idea to construct something in one place and perform some one line action in a separate method. I personally don't consider that comment as critical and we can live with it and we can put nit, but the goal of code reviews as well is to make sure that most engineers who work on that code base are in a consensus of what is going to land. We all work with it after all and have different views on how things should be done and written, but it's better to agree, change and move on if some place is easier to understand for others. Plus that kind of change is very fast to do, instead of spending time discussing it.
There was a problem hiding this comment.
I never mentioned that we should do this always construct something in one place and perform some one line action in a separate method nor that it is a bad practice, it depends.
This is just to separate the flow, maybe renaming the method would have been more clearer or a better choice. But, if the preference is to move the call along with the construction of object then will move.
There was a problem hiding this comment.
I agree that having a method called fetchUserAssetsAndDetails just call fetchAllAssetsAndDetails is very confusing. Pick one or the other, whichever makes more sense here
| finish(); | ||
| return; | ||
| } | ||
| } catch (ActivityNotFoundException ignored) { |
There was a problem hiding this comment.
This is very unlikely to happen, right?
Shall we consider calling finish() even inside the catch clause?
There was a problem hiding this comment.
We can though waiting for @SergeyZhukovsky to confirm we wanna start killing activities in case we don't have access to model objects.
There was a problem hiding this comment.
let's just keep as is for now. We have an issue to go away from the activity, we will figure it out eventually
| private void showAccounts(RecyclerView rvAccounts, WalletCoinAdapter walletCoinAdapter, | ||
| List<WalletListItemModel> walletListItemModelList) { | ||
| walletCoinAdapter.setWalletListItemModelList(walletListItemModelList); | ||
| walletCoinAdapter.setOnWalletListItemClick(AssetDetailActivity.this); |
There was a problem hiding this comment.
Nit: this should suffice.
| walletCoinAdapter.setOnWalletListItemClick(AssetDetailActivity.this); | ||
| walletCoinAdapter.setWalletListItemType(Utils.ACCOUNT_ITEM); | ||
| rvAccounts.setAdapter(walletCoinAdapter); | ||
| rvAccounts.setLayoutManager(new LinearLayoutManager(AssetDetailActivity.this)); |
There was a problem hiding this comment.
Nit: this should suffice.
| @@ -91,7 +92,7 @@ public boolean isFiatHistoryEmpty() { | |||
| } | |||
|
|
|||
| for (AssetTimePrice assetTimePrice : mFiatHistory) { | |||
| if (Double.valueOf(assetTimePrice.price) > 0.001d) { | |||
| if (Double.parseDouble(assetTimePrice.price) > 0.001d) { | |||
There was a problem hiding this comment.
Very unlikely to happen, but I would catch the two exceptions that may be thrown from parseDouble: NumberFormatException, and NullPointerException.
| .collect(toMap(i -> networks.get(i).chainId, Function.identity())); | ||
| } | ||
|
|
||
| public static class AssetAccountsNetworkBalance { |
There was a problem hiding this comment.
If this class is used only in PortfolioHelper I would reduce the visibility and make it private.
Would you please resolve the comments that have been addressed and create issues for the ones weren't covered within the scope of this PR/issues? like memory leaks etc. I am going to merge this. please address the above once you get a chance. |
Resolves brave/brave-browser#27333
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
all-networks.webm