Asset detail page clean up #253
Conversation
|
|
||
| return( | ||
| <div> | ||
| { assetTxs === null ? <p>{t`No Issuance History`}</p> |
There was a problem hiding this comment.
A null here would mean that the transactions haven't loaded yet, an empty array would indicate that there are no transactions (which shouldn't really be possible if the asset exists).
Better to show nothing (or more ideally, a loading indicator) rather than the user seeing "No Issuance History" flashing for a moment and disappearing.
| // Supply Starts at Zero | ||
| let supply = 0 | ||
| // Sort AssetTxs by block_time and Calculate Supply Change / Total Supply | ||
| assetTxs.sort((a,b) => (a.status.block_time > b.status.block_time) ? 1 : ((b.status.block_time > a.status.block_time) ? -1 : 0)) |
There was a problem hiding this comment.
The transactions are already sorted with newest first, if you want oldest first you can just reverse() it.
There was a problem hiding this comment.
I tried "assetTxs.reverse()" It wasn't working that's why I used ".sort"
There was a problem hiding this comment.
Wasn't working how? It didn't reverse the array?
There was a problem hiding this comment.
It didn't reverse the array. I tried console.log(assetTxs) and assetTxs.reverse() got the same array. No changes. :(
|
|
||
| // Find all Burn Transactions and Remove them from Supply | ||
| tx.vout.map(voutTx => { | ||
| if(voutTx.scriptpubkey_type === "op_return"){ |
There was a problem hiding this comment.
Similarly to issuances, this should also filter for outputs of the current asset id only.
| // Find all Issuance and add them to Supply | ||
| tx.vin.map(vinTx => { | ||
| if("issuance" in vinTx){ | ||
| calTxObj.supplyChange = chain_stats.has_blinded_issuances ? t`Confidential` : formatAssetAmount(("+ " + vinTx.issuance.assetamount), asset.precision, t) |
There was a problem hiding this comment.
formatAssetAmount expects a number, invoking it with the "+ " prefix won't work as expected.
We can have formatAssetAmount() accept an additional optional argument that specificies whether the sign should always be shown explicitly.
|
|
||
| // Find all Issuance and add them to Supply | ||
| tx.vin.map(vinTx => { | ||
| if("issuance" in vinTx){ |
There was a problem hiding this comment.
AFAIK, while the elements rpc doesn't support it, it is theoretically possible to create a single transaction with multiple issuances, which would confuse this. This should also check for the issued asset id.
client/src/app.js
Outdated
| @@ -334,6 +338,11 @@ export default function main({ DOM, HTTP, route, storage, scanner: scan$, search | |||
| ? { category: 'asset-txs', method: 'GET', path: `/asset/${d.asset_id}/txs/chain/${last(d.last_txids)}` } | |||
| : { category: 'asset-txs', method: 'GET', path: `/asset/${d.asset_id}/txs` }]) | |||
|
|
|||
| // Fetch Asset Icons | |||
| , !process.env.ASSET_MAP_URL ? O.empty() : O.of( | |||
| { category: 'asset-icons', method: 'GET', path: `https://assets.blockstream.info/icons.json` }) | |||
There was a problem hiding this comment.
I'm not sure if loading all the icons all the time is the right way to go at this. The green apps do this to enhance privacy, so the server can't learn which assets the user is interested in. But in a block explorer all privacy bets are off anyway, so we could just make them available as proper images that could be embedded via a URL, say https://assets.blockstream.info/icons/<assetid>.png.
Loading images individually would help reduce bandwidth. The icons.json file currently weights 1MB and is expected to continue growing. And it would also avoid base64 encoding, which requires 1.37x more bytes.
There was some discussion at Blockstream/asset_registry#26 about revamping the icons system, making them available as standalone files could be done as part of that.
(If we're keeping this based on the icons.json file, I would make the URL configurable via an ASSET_ICONS_URL environment variable, change the ternary condition to use that, and add a default value in flavors/liquid-mainnet/config.env.)
client/src/app.js
Outdated
| @@ -207,6 +207,10 @@ export default function main({ DOM, HTTP, route, storage, scanner: scan$, search | |||
| // In elements, we block rendering until the assetMap is loaded. Otherwise, we can start immediately. | |||
| , isReady$ = process.env.ASSET_MAP_URL ? assetMap$.mapTo(true).startWith(false) : O.of(true) | |||
|
|
|||
| // Asset Icons Response | |||
| , assetIcons$ = !process.env.ASSET_MAP_URL ? O.of({}) : | |||
There was a problem hiding this comment.
This is independent of ASSET_MAP_URL, should use IS_ELEMENTS or the ASSET_ICONS_URL configuration proposed below.
| }) | ||
| calculatedAssetTx.push(calTxObj) | ||
| }) | ||
| } |
There was a problem hiding this comment.
You could consider deriving calculatedAssetTx as a map() over assetTxs that transforms them into calTxObjs.
| <div className="assets-table-cell" data-label={t`Name`}>{asset.name}</div> | ||
| <div className="assets-table-cell" data-label={t`Name`}> | ||
| <div className="assets-table-name"> | ||
| {assetIcons === null ? "" : |
There was a problem hiding this comment.
Is it okay that the asset-icon-placeholder won't be added to the DOM while the icons are loading?
| @@ -45,208 +47,54 @@ export default ({ t, asset, assetTxs, goAsset, openTx, spends, tipHeight, loadin | |||
|
|
|||
| return layout( | |||
| <div> | |||
| <div className="jumbotron jumbotron-fluid asset-page"> | |||
| <div className="container back-nav"> | |||
| <a href="/assets/" className="back-link"><span>{`<`}</span>{`All Assets`}</a> | |||
There was a problem hiding this comment.
Links shouldn't start with a leading /, this will break when esplora is served from a subdirectory and not from the root of the hostname. There's a <base href> tag that makes links relative to the base url.
There was a problem hiding this comment.
`All Assets` should use t so it'll get translated.
adebayooriyomi
force-pushed
the
asset_detail_page_clean_up
branch
from
7023429 to
f143870
Compare
Clean Up Asset Detail Page
Issue created by Allen
https://gl.blockstream.com/greenaddress/esplora/-/issues/97