Billy/454 tx info #770

okwme · 2018-05-30T15:25:59Z

Closes #454

Description:
the block explorer doesn't show any details of transactions.

followed the same method of querying txs from the explorer and added an rpc handler to get them.
need help with layout design of how these should look @nylira @jolesbi

will add tests next as this is still WIP

faboweb · 2018-05-30T15:29:48Z

app/src/renderer/connectors/lcdClient.js

@@ -89,6 +89,8 @@ Object.assign(Client.prototype, {
  },
  coinTxs: argReq("GET", "/tx/coin"),

+  txs: argReq("GET", "/txs"),


codecov · 2018-05-30T15:30:30Z

Codecov Report

Merging #770 into develop will decrease coverage by <.01%.
The diff coverage is 95.65%.

@@             Coverage Diff             @@
##           develop     #770      +/-   ##
===========================================
- Coverage    87.56%   87.55%   -0.01%     
===========================================
  Files           94       95       +1     
  Lines         1608     1639      +31     
  Branches        91       94       +3     
===========================================
+ Hits          1408     1435      +27     
- Misses         184      188       +4     
  Partials        16       16

Impacted Files	Coverage Δ
app/src/renderer/components/wallet/PageSend.vue	`93.33% <ø> (-0.15%)`	⬇️
app/src/renderer/vuex/getters.js	`100% <ø> (ø)`	⬆️
app/src/renderer/connectors/lcdClient.js	`95.23% <ø> (ø)`	⬆️
app/src/renderer/connectors/rpcWrapperMock.js	`100% <100%> (ø)`	⬆️
app/src/renderer/vuex/modules/blockchain.js	`100% <100%> (+2.32%)`	⬆️
app/src/renderer/vuex/modules/wallet.js	`88.46% <100%> (ø)`	⬆️
app/src/renderer/scripts/tx-utils.js	`100% <100%> (ø)`
app/src/renderer/components/monitor/PageBlock.vue	`81.25% <75%> (-18.75%)`	⬇️
... and 2 more

jbibla · 2018-05-30T17:32:10Z

i'm not sure about what data is actually available - but if it could match the tx history component that might be nice.

…lly/454-tx-info

faboweb · 2018-06-04T14:04:08Z

app/src/renderer/components/monitor/PageBlock.vue

@@ -44,13 +44,23 @@ page(:title="pageBlockTitle")
    part(title='Transactions')
      data-loading(v-if="blockchain.blockLoading")
      data-empty(v-else-if="block.header.num_txs === 0" title="Empty Block" subtitle="There were no transactions in this block.")
-      list-item(v-else v-for="tx in block.data.txs" :key="tx.id" dt="Transaction" :dd="TODO")
+      template(


@jolesbi this is what you were looking for, right?

faboweb · 2018-06-04T14:04:46Z

app/src/renderer/components/monitor/PageBlock.vue

@@ -68,12 +79,15 @@ export default {
    Page
  },
  computed: {
-    ...mapGetters(["blockchain"]),
+    ...mapGetters(["blockchain", "blocTxInfo", "config"]),


at least it was consistently misspelled 😜

faboweb · 2018-06-04T14:07:31Z

app/src/renderer/vuex/modules/blockchain.js

@@ -61,6 +81,66 @@ export default ({ commit, node }) => {
        })
      })
    },
+    async queryTxInfo({ state, dispatch }, height) {
+      let blockTxInfo = state.blockTxs.find(


why not store them per height?

was following method used by queryBlockInfo() but you're right keying the height is smart i'll update both of em

actually just updating the blockchain one for sake of PR scope

actually i think both of these need to stay as arrays as they are used in for loops

Can you point me to where they are used in loops? What I have found, there is always a selection by height happening when blockTxs appear in code.

oops i totally misinterpreted the unit test error relating to should disable the next block button if last block. thought that info was counting an array length for looping and failed when it became an obj (as per a similar error in blockchain.spec.js). i've gone back and updated to a keyed storage method for blockTxs as well as blockMetas

faboweb · 2018-06-04T14:08:55Z

app/src/renderer/vuex/modules/blockchain.js

+        blockTxInfo && state.blockTxs.push(blockTxInfo)
+        return blockTxInfo
+      } catch (error) {
+        return Promise.reject(error)


isn't that the default for async functions rejecting on error?

i was having trouble getting the error reporting come out correctly in the tests so went through and made each error very explicit until i found the culprit, i'll walk it back

faboweb · 2018-06-04T14:09:37Z

app/src/renderer/vuex/modules/blockchain.js

+    },
+    getTxs({ state, commit, dispatch }, { key, len, txs }) {
+      return new Promise(async (resolve, reject) => {
+        if (key >= len) return resolve(txs)


Could you add a comment stating what this statement accomplishes?

faboweb · 2018-06-04T14:10:24Z

app/src/renderer/vuex/modules/blockchain.js

+        return Promise.reject(error)
+      }
+    },
+    convertTx({ state }, txstring) {


this doesn't do any manipulation on the store and should therefor maybe be in a script or someplace

is there a clever place to put it in or just make a new file under /src/scripts?

faboweb · 2018-06-04T14:11:31Z

app/src/renderer/vuex/modules/wallet.js

@@ -84,9 +85,9 @@ export default ({ commit, node }) => {
    },
    async queryWalletHistory({ state, commit, dispatch }) {
      commit("setHistoryLoading", true)
-      // let res = await node.coinTxs(state.address)
+      let res = await node.coinTxs(state.address)


does history work again?!

oh i uncommented this to see what NiLiTx looked like and it seemed to work with mock network. i'll remove tho since that's not the point of this pr

faboweb · 2018-06-04T19:42:36Z

app/src/renderer/scripts/utils.js

+import varint from "varint"
+import b64 from "base64-js"
+
+export default function(txstring) {


The script is called utils and exports a function that converts tx strings into json? I think this is not really understandable.
How about a script called tx-utils.js that exports a function named parseTx?

I agree it should be named, could be something like getTxHash

makes sense, i think getTxHash is an accurate name

faboweb · 2018-06-04T19:50:21Z

app/src/renderer/vuex/modules/blockchain.js

+        ])
+          .then(() => {
+            state.blockLoading = false
+            dispatch("queryTxInfo", height)


If you return this, I think you save yourself the inner .catch

refactored all the promises to be more concise

mappum

Left some comments but nice work overall, I know it can be a pain figuring out how to derive the same hash as the golang code. 😋

mappum · 2018-06-05T01:55:34Z

app/src/renderer/scripts/utils.js

+import b64 from "base64-js"
+
+export default function(txstring) {
+  let txbytes = b64.toByteArray(txstring)


Why add a dependency instead of doing Buffer.from(base64string, 'base64')?

didn't realize it was that ez!

mappum · 2018-06-05T02:09:01Z

app/src/renderer/vuex/modules/blockchain.js

+        let txstring = atob(txs[key])
+        let hash = await convertTx(txs[key])
+        node
+          .txs(hash)


Why mix await and .then syntax?

refactored all the promises to be more concise

mappum · 2018-06-05T02:11:08Z

app/src/renderer/vuex/modules/blockchain.js

+        blockTxInfo = await dispatch("getTxs", {
+          key: 0,
+          len:
+            state.block && state.block.data && state.block.data.txs


What's the purpose of these chains of &&'s? Seems like it makes things harder to debug because if the format changes or data is missing, it will silently pass this call and have undefined behavior inside getTxs.

this was to allow the tests that had very minimal dummy blocks to pass. but you're right if the block is malformed it should throw an error. i've updated the other tests to use a well formed block dummy

mappum · 2018-06-05T02:14:13Z

app/src/renderer/scripts/utils.js

@@ -0,0 +1,14 @@
+import createHash from "create-hash"


Couldn't we just get this from the Node crypto module to avoid adding a dependency?

nice, didn't realize it was also that ez

mappum · 2018-06-05T02:14:52Z

app/src/renderer/scripts/utils.js

+import varint from "varint"
+import b64 from "base64-js"
+
+export default function(txstring) {


I agree it should be named, could be something like getTxHash

okwme added 4 commits May 30, 2018 15:23

pause

09fa294

txs loading, need styles and tests

0f799d6

test fix

171b219

changelog

ae37dc7

okwme requested review from faboweb, NodeGuy and nylira as code owners May 30, 2018 15:25

faboweb reviewed May 30, 2018

View reviewed changes

okwme and others added 11 commits June 1, 2018 14:49

Merge branch 'develop' into billy/454-tx-info

a2d926b

wait til network back online

79d0d76

pause

a05d8ba

Merge branch 'develop' into billy/454-tx-info

d0c3184

networking

5189ebc

Merge branch 'billy/454-tx-info' of github.com:cosmos/voyager into bi…

eb783f0

…lly/454-tx-info

testnet bad replies

933b571

enough coverage?

a5299cc

Merge branch 'develop' into billy/454-tx-info

2408361

Merge branch 'develop' into billy/454-tx-info

7c82076

Merge branch 'billy/454-tx-info' of github.com:cosmos/voyager into bi…

57d3a35

…lly/454-tx-info

faboweb changed the title ~~WIP: Billy/454 tx info~~ Billy/454 tx info Jun 4, 2018

faboweb reviewed Jun 4, 2018

View reviewed changes

fabo feedback

9965edf

okwme added 4 commits June 4, 2018 17:44

fixed test

b016e6b

loops

945fb68

make utils file

d2ed57f

remove block history

3eeeec3

faboweb reviewed Jun 4, 2018

View reviewed changes

mappum reviewed Jun 5, 2018

View reviewed changes

okwme added 5 commits June 5, 2018 11:34

store txs keyed by height

e3fcb7c

metaBlocks keyed by height

94c167f

clean up promises and tx util

1373730

further matt feedback

86e36e7

Merge branch 'develop' into billy/454-tx-info

87b1375

nylira approved these changes Jun 5, 2018

View reviewed changes

okwme merged commit 604b15f into develop Jun 5, 2018

okwme deleted the billy/454-tx-info branch June 5, 2018 15:47

Billy/454 tx info #770

Billy/454 tx info #770

Conversation

okwme commented May 30, 2018

Choose a reason for hiding this comment

codecov bot commented May 30, 2018 • edited Loading

Codecov Report

jbibla commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mappum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 30, 2018 •

edited

Loading