-
Notifications
You must be signed in to change notification settings - Fork 909
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
Add GetEthTokenSymbol and GetEthTokenDecimals to JsonRpcService #18254
Conversation
2c31d72
to
8c1efdf
Compare
91df724
to
f9b176b
Compare
RequestInternal(eth::eth_call("", contract_address, "", "", "", data, | ||
kEthereumBlockTagLatest), | ||
true, network_url, std::move(internal_callback)); | ||
} |
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.
Note - Jocelyn had asked that GetEthTokenSymbol and GetEthTokenDecimals share the code since they are similar, however because symbol()
and decimals()
return different types (string and uint8 respectively), their parsing logic is different, so I didn't see much opportunity for reuse.
|
||
// Fetches decimals of an ERC20 or ERC721 token. | ||
GetEthTokenDecimals(string chain_id, | ||
string contract_address) => (string symbol, |
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.
symbol->decimals
auto internal_callback = | ||
base::BindOnce(&JsonRpcService::OnGetEthTokenDecimals, | ||
weak_ptr_factory_.GetWeakPtr(), std::move(callback)); | ||
RequestInternal(eth::eth_call("", contract_address, "", "", "", data, |
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.
Short form please: eth_call(contract_address, data)
for both methods
"result":"$1" | ||
})"; | ||
|
||
std::string formatJsonRpcResponse(const std::string& value) { |
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.
formatJsonRpcResponse->FormatJsonRpcResponse
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 with minor suggestions
* Rename symbol -> decimals * Rename formatJsonRpcResponse -> FormatJsonRpcResponse * Use short form of eth::eth_call
f9b176b
to
0106405
Compare
Resolves brave/brave-browser#29983
Exposes two APIs to the frontend, GetEthTokenSymbol and GetEthTokenDecimals, so that this data can be fetched. The first use case is for swaps with unknown tokens, however we can also use this to pre-fetch symbols when users are adding NFTs manually so they do not need to input it themselves.
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: