BigInt basics + balance sheet report #11
Conversation
sophialittlejohn
changed the title
sophialittlejohn
changed the title
src/utils/BigInt.ts
Outdated
| return this._add<T>(-val) | ||
| } | ||
|
|
||
| _mul<T>(value: bigint | (T extends BigIntWrapper ? T : never)): T { |
There was a problem hiding this comment.
I think these will only be useful if they operate on the decimal representation. We may want something like currency = tokens.mul(price). I think it should be multiplying by e.g. 1.1234 and not by 1.1234e18
There was a problem hiding this comment.
Good point, I added a case to Currency.mul(), if a Price is passed to the multiply function it will divide by the Prices decimals and then multiply. We could optionally also expose the _mulPrice function to make it more clear
| toFloat() { | ||
| return this.toDecimal().toNumber() | ||
| } | ||
|
|
There was a problem hiding this comment.
Nice to have would be to also have gt, lt and eq
src/Pool.ts
Outdated
| reports() { | ||
| return new Reports(this._root, this.id) | ||
| } |
There was a problem hiding this comment.
I think it should either be a query or a property. Property makes the most sense I think. await pool.reports.balanceSheet()
src/Reports/index.ts
Outdated
| }), | ||
| ]) | ||
|
|
||
| return ReportService.generate(processors.balanceSheet, this.poolId, { poolSnapshots, trancheSnapshots }, filter) |
There was a problem hiding this comment.
I think the ReportService does more harm than good. The poolSnapshots, trancheSnapshots queries are already cached, so it's really only memoizing the process function. If poolSnapshots or trancheSnapshots return new data, this wouldn't necessarily be reflected in the result, because the ReportService might have a cached result. I would just call the process function directly.
There was a problem hiding this comment.
Can you please elaborate how you think the ReportService does harm? It seems like you're mostly just addressing the caching ability?
The cache is only valid for 5 minutes and it auto cleared after, I can reduce the time. And you got it right, the process function is exactly the one I'm trying to avoid being called n times every time its requested. I think the cache becomes even more relevant as the data set grows. We could be processing hundreds of thousands of data points multiple times as the user clicks into a graph (and switches in between graphs)
From a UX perspective we shouldn't just be updating data in a table without the user noticing. What we should do is let the user know that fresh data is available so they can click to update when ready. Kind of like how when you're reviewing PR and someone pushes something new
There was a problem hiding this comment.
Maybe we could implement a strategy that would invalidate the cache if there's new data available from the subquery
There was a problem hiding this comment.
Yes that would be the way to go. You'd want a "useMemo" and not a "useQuery". However, neither is necessary, because once the body of this function gets wrapped in a _query, you can pass the cacheKey to that and the result will be cached. Then the process function won't be called more than once, when it receives the same filters and the data is still the same.
The UX idea is a nice one and we can definitely take that into account when we start working on hooks and integrating into the App, but that's a userland problem. The SDK just provides the queries that users can await or subscribe to. They can choose how to display it and how the UI responds to updates.
We could, for example, have a hook that queues new data and waits for user interaction before returning the new data.
const { data, hasFreshData, refresh } = useObservableWithRefresh(...)
There was a problem hiding this comment.
You haven't elaborated how the caching does more harm than good, would be great if you could provide some clarity
There was a problem hiding this comment.
The UX idea is a nice one and we can definitely take that into account when we start working on hooks and integrating into the App, but that's a userland problem. The SDK just provides the queries that users can await or subscribe to. They can choose how to display it and how the UI responds to updates.
while it does sound like a hooks challenge, to me it's also a problem we want to solve in the SDK first, it will influence how we push data updates through to the end user of the SDK
src/Reports/index.ts
Outdated
| return ReportService.generate(processors.balanceSheet, this.poolId, { poolSnapshots, trancheSnapshots }, filter) | ||
| } | ||
|
|
||
| async poolSnapshots(filter?: PoolSnapshotFilter) { |
There was a problem hiding this comment.
| async poolSnapshots(filter?: PoolSnapshotFilter) { | |
| poolSnapshots(filter?: PoolSnapshotFilter) { |
src/Reports/index.ts
Outdated
| } | ||
|
|
||
| async poolSnapshots(filter?: PoolSnapshotFilter) { | ||
| return this._root._queryCentrifugeApi(['poolSnapshots'], poolSnapshotsQuery, { filter }, poolSnapshotsPostProcess) |
There was a problem hiding this comment.
I pushed a change to the queries branch to get rid of the key argument. The result will just be cached by the graphql query and variables
|
converting back to draft to work on websocket implementation |
sophialittlejohn
force-pushed
the
bigint-types
branch
from
d2455ae to
88c77ff
Compare
Description
This pull request...
Tests:
BigInt.test.tswrapper tests: conversion and arithmeticReport.test.tsBalance sheet report generation#15