-
Notifications
You must be signed in to change notification settings - Fork 894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wallet address resolution for Android #16423
Conversation
if (WalletNativeUtils.isUnstoppableDomainsTld(domain)) { | ||
mJsonRpcService.unstoppableDomainsGetWalletAddr( | ||
domain, mCurrentBlockchainToken, (response, errorResponse, errorString) -> { | ||
onResolveWalletAddressDone(domain, response); | ||
}); | ||
return true; | ||
} | ||
|
||
if (mCurrentBlockchainToken.coin == CoinType.ETH && WalletNativeUtils.isEnsTld(domain)) { | ||
mJsonRpcService.ensGetEthAddr(domain, null, | ||
(response, requireOffchainConsent, errorResponse, errorString) -> { | ||
onResolveWalletAddressDone(domain, response); | ||
}); | ||
return true; | ||
} | ||
|
||
if (ChromeFeatureList.isEnabled(BraveFeatureList.BRAVE_WALLET_SNS)) { | ||
if (mCurrentBlockchainToken.coin == CoinType.SOL | ||
&& WalletNativeUtils.isSnsTld(domain)) { | ||
mJsonRpcService.snsGetSolAddr(domain, (response, errorResponse, errorString) -> { | ||
onResolveWalletAddressDone(domain, response); | ||
}); | ||
return true; | ||
} | ||
} | ||
|
||
mResolvedAddrText.setVisibility(View.GONE); | ||
mResolvedAddrText.setText(""); | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually will be refactored to a single mojo call brave/brave-browser#26655
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
browser/net LGTM
|
if (result == null || result.isEmpty()) { | ||
mResolvedAddrText.setVisibility(View.GONE); | ||
mResolvedAddrText.setText(""); | ||
String notRegsteredErrorText = String.format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notRegsteredErrorText -> notRegisteredErrorText
} | ||
|
||
if (ChromeFeatureList.isEnabled(BraveFeatureList.BRAVE_WALLET_SNS)) { | ||
if (mCurrentBlockchainToken.coin == CoinType.SOL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be combined to a single if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supposed to be a temporary feature and to be removed as soon as our backend is ready. While inner if block matches pattern above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++. There are two minor suggestions, but other than that looks good. Thanks for making it for Android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolves brave/brave-browser#27464
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
untitled.webm