[WIP] Implement BalanceView #3

Closed
rohfosho wants to merge 6 commits from ro/token-balances into main
rohfosho commented 2022-04-08 12:16:31 -04:00 (Migrated from github.com)

This PR implements the BalanceView and the required logic to fetch and display balances for Ethereum and the supported ERC20s for a given address.

  • Android
  • Darwin
  • Web
This PR implements the BalanceView and the required logic to fetch and display balances for Ethereum and the supported ERC20s for a given address. - [x] Android - [ ] Darwin - [ ] Web
conradev (Migrated from github.com) requested changes 2022-04-08 22:01:29 -04:00
conradev (Migrated from github.com) left a comment

Left a bunch of comments – mostly small stuff. This looks great!

Left a bunch of comments – mostly small stuff. This looks great!
@ -0,0 +32,4 @@
.fillMaxSize()
.padding(16.dp)
.verticalScroll(rememberScrollState()),
horizontalAlignment = Alignment.CenterHorizontally
conradev (Migrated from github.com) commented 2022-04-08 21:05:34 -04:00

Formatting is off seemingly

        Column(
            modifier = Modifier
                .fillMaxWidth()
                .fillMaxSize()
                .padding(16.dp)
                .verticalScroll(rememberScrollState()),
            horizontalAlignment = Alignment.CenterHorizontally
        )
Formatting is off seemingly ```suggestion Column( modifier = Modifier .fillMaxWidth() .fillMaxSize() .padding(16.dp) .verticalScroll(rememberScrollState()), horizontalAlignment = Alignment.CenterHorizontally ) ```
@ -0,0 +55,4 @@
@Composable
fun BalanceViewPreview() {
BalanceView(PreviewMocks.get())
}
conradev (Migrated from github.com) commented 2022-04-08 21:06:29 -04:00

No newline at the end of the file – run ./gradlew lintKotlin to see lint errors or ./gradlew formatKotlin to fix them

No newline at the end of the file – run `./gradlew lintKotlin` to see lint errors or `./gradlew formatKotlin` to fix them
@ -0,0 +35,4 @@
text = balance.displayName,
textAlign = TextAlign.Start,
fontWeight = FontWeight.SemiBold,
fontSize = 18.sp
conradev (Migrated from github.com) commented 2022-04-08 21:10:45 -04:00

Instead of manually specifying the weight and size, I would see if you can find a style from MaterialTheme that matches:

                style = MaterialTheme.typography.something
Instead of manually specifying the weight and size, I would see if you can find a style from `MaterialTheme` that matches: ```suggestion style = MaterialTheme.typography.something ```
@ -0,0 +41,4 @@
horizontalAlignment = Alignment.End,
) {
Text(
text = "\$TODO",
conradev (Migrated from github.com) commented 2022-04-08 21:11:55 -04:00

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

                    text = "\$0.00",
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 ```suggestion text = "\$0.00", ```
@ -0,0 +15,4 @@
data class Balance(
val address: Address,
val contractAddress: Address?,
val quantity: Quantity
conradev (Migrated from github.com) commented 2022-04-08 21:15:41 -04:00

Perhaps we should consider putting timestamp on Balance, 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

Perhaps we should consider putting `timestamp` on `Balance`, 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(
conradev (Migrated from github.com) commented 2022-04-08 21:13:09 -04:00
internal fun EthereumBalanceRecord.toBalance() = Balance(
```suggestion internal fun EthereumBalanceRecord.toBalance() = Balance( ```
@ -30,1 +47,4 @@
.let { LocalDateTime.parse(it) }
}
internal fun Database.Companion.invoke(driver: SqlDriver): Database {
conradev (Migrated from github.com) commented 2022-04-08 21:18:42 -04:00

I'd consider storing Address as a blob, just like we do for Quantity

I'd consider storing `Address` as a blob, just like we do for `Quantity`
@ -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) {
conradev (Migrated from github.com) commented 2022-04-08 21:22:54 -04:00

Do we still need this? I'd imagine we shouldn't be doing hex decoding in the view code (only the ViewModel code)

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)
}
conradev (Migrated from github.com) commented 2022-04-08 21:26:58 -04:00

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:

internal interface BalanceUpdater {
    val scope: CoroutineScope
    suspend fun update(address: Address)
    
    fun update(address: Address) {
        scope.launch {
             update(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: ```suggestion internal interface BalanceUpdater { val scope: CoroutineScope suspend fun update(address: Address) fun update(address: Address) { scope.launch { update(address) } } } ```
@ -0,0 +18,4 @@
fun update(address: Address)
}
internal class EthereumBalanceUpdater(
conradev (Migrated from github.com) commented 2022-04-08 21:31:14 -04:00

I've been naming the concrete implementations of interfaces after how they do it, i.e. DatabaseAccountStore stores accounts using the database

This class updates balances using RpcClient, so perhaps RpcClientBalanceUpdater? It also does so for Tokens, so maybe RpcTokenBalanceUpdater?

I've been naming the concrete implementations of interfaces after _how_ they do it, i.e. `DatabaseAccountStore` _stores_ accounts using the database This class _updates_ balances using `RpcClient`, so perhaps `RpcClientBalanceUpdater`? It also does so for `Token`s, so maybe `RpcTokenBalanceUpdater`?
@ -0,0 +24,4 @@
) : BalanceUpdater {
private val scope = CoroutineScope(EmptyCoroutineContext)
private val balanceOfABI = Keccak256Digest().digest("balanceOf(address)".toByteArray()).encodeHex()
conradev (Migrated from github.com) commented 2022-04-08 21:33:06 -04:00

This doesn't use this, so do you want to put it on companion object?

This doesn't use `this`, so do you want to put it on `companion object`?
@ -0,0 +48,4 @@
Balance(address = address, contractAddress = contractAddress, quantity = result)
.let { balanceStore.add(it) }
}
}
conradev (Migrated from github.com) commented 2022-04-08 21:42:36 -04:00

Right now this updates the balances sequentially – I think we should update them in parallel:

    private suspend fun updateTokenBalance(token: Token address: Address) {
        val tx = Transaction(
            to = token.contractAddress,
            data = Data.fromString(
                "0x${balanceOfABI}000000000000000000000000${
                    address.toString().drop(2)
                }"
            ),
        )

        val result = rpcClient.request(Call<Quantity>(tx))
        val balance = Balance(address = address, contractAddress = contractAddress, quantity = result)
        balanceStore.add(balance)
    }
    
    private suspend fun updateTokenBalances(address: Address) {
        val requests = Token.values().map { token ->
            async { updateTokenBalance(token, address) }
        }
        awaitAll(requests)
    }

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

Right now this updates the balances sequentially – I think we should update them in parallel: ```suggestion private suspend fun updateTokenBalance(token: Token address: Address) { val tx = Transaction( to = token.contractAddress, data = Data.fromString( "0x${balanceOfABI}000000000000000000000000${ address.toString().drop(2) }" ), ) val result = rpcClient.request(Call<Quantity>(tx)) val balance = Balance(address = address, contractAddress = contractAddress, quantity = result) balanceStore.add(balance) } private suspend fun updateTokenBalances(address: Address) { val requests = Token.values().map { token -> async { updateTokenBalance(token, address) } } awaitAll(requests) } ``` Also, using ERC20 in the method name takes away from the clarity, because we call them `Token`s 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) }
conradev (Migrated from github.com) commented 2022-04-08 21:44:48 -04:00

I don't love this usage of let – the variable binding (val balance = ) adds some nice clarity and is easier to read IMO

I love using let when returning a value, when chaining them, and when dealing with optionals, but here it just feels excessive

I don't love this usage of let – the variable binding (`val balance = `) adds some nice clarity and is easier to read IMO I love using `let` when 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)
conradev (Migrated from github.com) commented 2022-04-08 21:47:27 -04:00
    singleOf(::EthereumBalanceStore) bind BalanceStore::class
```suggestion singleOf(::EthereumBalanceStore) bind 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)
conradev (Migrated from github.com) commented 2022-04-08 21:48:20 -04:00

Interesting, factoryOf wasn't working here?

    factoryOf(::EthereumBalanceUpdater) bind BalanceUpdater::class
Interesting, `factoryOf` wasn't working here? ```suggestion factoryOf(::EthereumBalanceUpdater) bind BalanceUpdater::class ```
@ -20,0 +32,4 @@
}
internal class MockBalanceUpdater() : BalanceUpdater {
override fun update(address: Address) = Unit
conradev (Migrated from github.com) commented 2022-04-08 21:49:44 -04:00

I'd consider putting some mock data here so we can see a row or two in the preview

I'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) }
conradev (Migrated from github.com) commented 2022-04-08 21:53:37 -04:00

There is not much point to using a Map if we construct it every time (first would be faster)

        private val contractAddressToToken: Map<Address, Token> by lazy {
            values().associate { (it.contractAddress to it) }
        }
There is not much point to using a `Map` if we construct it every time (`first` would be faster) ```suggestion private val contractAddressToToken: Map<Address, Token> by lazy { values().associate { (it.contractAddress to it) } } ```
@ -0,0 +18,4 @@
import kotlin.math.pow
import kotlin.math.roundToInt
fun Double.toString(decimals: Int) : String {
conradev (Migrated from github.com) commented 2022-04-08 21:54:35 -04:00
internal fun Double.toString(decimals: Int) : String {
```suggestion internal fun Double.toString(decimals: Int) : String { ```
@ -0,0 +39,4 @@
token.displayName,
token.symbol,
balanceString(quantity, token.decimals, token.symbol)
)
conradev (Migrated from github.com) commented 2022-04-08 21:57:25 -04:00

I was being a sillypants – we should just move this to fun address down below

I was being a sillypants – we should just move this to `fun address` down below
@ -0,0 +48,4 @@
.toString(decimals)
.let { "$it $symbol" }
fun eth(quantity: Quantity): RowViewModel = RowViewModel(
conradev (Migrated from github.com) commented 2022-04-08 21:56:23 -04:00
            fun eth(quantity: Quantity) = RowViewModel(
```suggestion fun eth(quantity: Quantity) = RowViewModel( ```
@ -0,0 +61,4 @@
}
}
private val viewModelScope = CoroutineScope(EmptyCoroutineContext)
conradev (Migrated from github.com) commented 2022-04-08 21:58:45 -04:00

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 coroutineScope simply because Koin scopes are a thing, but unless there is a Koin scope on this class, scope seems fine

    private val scope = 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 `coroutineScope` simply because `Koin` scopes are a thing, but unless there is a `Koin` scope on this class, `scope` seems fine ```suggestion private val scope = CoroutineScope(EmptyCoroutineContext) ```
@ -0,0 +72,4 @@
.collectLatest { ethereumBalanceUpdater.update(it) }
}
}
conradev (Migrated from github.com) commented 2022-04-08 22:00:20 -04:00

I'd add this to save yourself all of the repetition

val ethereumAddress: Flow<Address> = accountStore.accountsFlow
     .map { it.first() }
     .map { it.ethereumAddress }
I'd add this to save yourself all of the repetition ```kotlin val ethereumAddress: Flow<Address> = accountStore.accountsFlow .map { it.first() } .map { it.ethereumAddress } ```
@ -0,0 +10,4 @@
insert:
INSERT INTO ethereum_balance(address, contract_address, balance, timestamp)
VALUES (?, ?, ?, datetime('now'))
conradev (Migrated from github.com) commented 2022-04-08 21:21:52 -04:00

If we do this, we can delete the "T" replacing code above

VALUES (?, ?, ?, strftime('%Y-%m-%dT%H:%M:%S', 'now'))
If we do this, we can delete the "T" replacing code above ```suggestion VALUES (?, ?, ?, strftime('%Y-%m-%dT%H:%M:%S', 'now')) ```
@ -0,0 +19,4 @@
WHERE address = ?
ORDER BY balance, contract_address;
balanceFor:
conradev (Migrated from github.com) commented 2022-04-08 22:01:10 -04:00

If we don't use this, I'd nix it

If we don't use this, I'd nix it
conradev commented 2022-04-09 20:08:47 -04:00 (Migrated from github.com)

also, we don't have a web UI at the moment

  • Web
also, we don't have a web UI at the moment > * [ ] Web

Pull request closed

Sign in to join this conversation.
No description provided.