[WIP] Implement BalanceView #3
No reviewers
Labels
No labels
bug
codex
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
conrad/Wallet!3
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "ro/token-balances"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR implements the BalanceView and the required logic to fetch and display balances for Ethereum and the supported ERC20s for a given address.
Left a bunch of comments – mostly small stuff. This looks great!
@ -0,0 +32,4 @@.fillMaxSize().padding(16.dp).verticalScroll(rememberScrollState()),horizontalAlignment = Alignment.CenterHorizontallyFormatting is off seemingly
@ -0,0 +55,4 @@@Composablefun BalanceViewPreview() {BalanceView(PreviewMocks.get())}No newline at the end of the file – run
./gradlew lintKotlinto see lint errors or./gradlew formatKotlinto fix them@ -0,0 +35,4 @@text = balance.displayName,textAlign = TextAlign.Start,fontWeight = FontWeight.SemiBold,fontSize = 18.spInstead of manually specifying the weight and size, I would see if you can find a style from
MaterialThemethat matches:@ -0,0 +41,4 @@horizontalAlignment = Alignment.End,) {Text(text = "\$TODO",I'd maybe use
$0.00(here and elsewhere) so that it is a bit more clear how the final version will look and is less jarring@ -0,0 +15,4 @@data class Balance(val address: Address,val contractAddress: Address?,val quantity: QuantityPerhaps we should consider putting
timestamponBalance, so that we can use it from the view model in the future. That would mean the RPC client has to add a timestamp, though, when parsing the models. Perhaps it can have a default value in the constructor@ -0,0 +20,4 @@internal val ETHEREUM_ADDRESS = Address.fromString("0x0000000000000000000000000000000000000000")internal fun EthereumBalanceRecord.toBalance() : Balance = Balance(@ -30,1 +47,4 @@.let { LocalDateTime.parse(it) }}internal fun Database.Companion.invoke(driver: SqlDriver): Database {I'd consider storing
Addressas a blob, just like we do forQuantity@ -10,3 +10,3 @@internal fun String.decodeHex(allowNibbles: Boolean = false): ByteArray {fun String.decodeHex(allowNibbles: Boolean = false): ByteArray {val string = when (length % 2 to allowNibbles) {Do we still need this? I'd imagine we shouldn't be doing hex decoding in the view code (only the ViewModel code)
@ -0,0 +16,4 @@internal interface BalanceUpdater {fun update(address: Address)}If a client wanted to wait for the update to be finished, they would be unable to with this interface. I'd suggest maybe doing this to get the best of both worlds:
@ -0,0 +18,4 @@fun update(address: Address)}internal class EthereumBalanceUpdater(I've been naming the concrete implementations of interfaces after how they do it, i.e.
DatabaseAccountStorestores accounts using the databaseThis class updates balances using
RpcClient, so perhapsRpcClientBalanceUpdater? It also does so forTokens, so maybeRpcTokenBalanceUpdater?@ -0,0 +24,4 @@) : BalanceUpdater {private val scope = CoroutineScope(EmptyCoroutineContext)private val balanceOfABI = Keccak256Digest().digest("balanceOf(address)".toByteArray()).encodeHex()This doesn't use
this, so do you want to put it oncompanion object?@ -0,0 +48,4 @@Balance(address = address, contractAddress = contractAddress, quantity = result).let { balanceStore.add(it) }}}Right now this updates the balances sequentially – I think we should update them in parallel:
Also, using ERC20 in the method name takes away from the clarity, because we call them
Tokens in the code. The methods to work with the classes should use the same naming as the classes, ideally@ -0,0 +55,4 @@val result = rpcClient.request(request)Balance(address = address, contractAddress = null, quantity = result).let { balanceStore.add(it) }I don't love this usage of let – the variable binding (
val balance =) adds some nice clarity and is easier to read IMOI love using
letwhen returning a value, when chaining them, and when dealing with optionals, but here it just feels excessive@ -29,0 +18,4 @@internal val sharedModule = module {singleOf(::KeyStore)single<AccountStore> { DatabaseAccountStore(get(), get()) }singleOf(::EthereumBalanceStore) binds arrayOf(BalanceStore::class)@ -39,3 +32,2 @@factory { RpcClient(get<RpcProvider>().endpointUrl) }factory { params -> KotlinLogging.logger(params.get<String>()) }factory { EthereumBalanceUpdater(get(), get()) } binds arrayOf(BalanceUpdater::class)Interesting,
factoryOfwasn't working here?@ -20,0 +32,4 @@}internal class MockBalanceUpdater() : BalanceUpdater {override fun update(address: Address) = UnitI'd consider putting some mock data here so we can see a row or two in the preview
@ -0,0 +34,4 @@companion object {private val contractAddressToToken: Map<Address, Token>get() = values().associate { (it.contractAddress to it) }There is not much point to using a
Mapif we construct it every time (firstwould be faster)@ -0,0 +18,4 @@import kotlin.math.powimport kotlin.math.roundToIntfun Double.toString(decimals: Int) : String {@ -0,0 +39,4 @@token.displayName,token.symbol,balanceString(quantity, token.decimals, token.symbol))I was being a sillypants – we should just move this to
fun addressdown below@ -0,0 +48,4 @@.toString(decimals).let { "$it $symbol" }fun eth(quantity: Quantity): RowViewModel = RowViewModel(@ -0,0 +61,4 @@}}private val viewModelScope = CoroutineScope(EmptyCoroutineContext)Considering this class is a view model, and there is only a single scope, I don't think we need the prefix.
I could see
coroutineScopesimply becauseKoinscopes are a thing, but unless there is aKoinscope on this class,scopeseems fine@ -0,0 +72,4 @@.collectLatest { ethereumBalanceUpdater.update(it) }}}I'd add this to save yourself all of the repetition
@ -0,0 +10,4 @@insert:INSERT INTO ethereum_balance(address, contract_address, balance, timestamp)VALUES (?, ?, ?, datetime('now'))If we do this, we can delete the "T" replacing code above
@ -0,0 +19,4 @@WHERE address = ?ORDER BY balance, contract_address;balanceFor:If we don't use this, I'd nix it
also, we don't have a web UI at the moment
Pull request closed