feat: new cli commands `accounts:notes` and `accounts:transactions` #1086

wd021 · 2022-02-28T14:07:02Z

Proposing to add an accounts:transactions command to display transaction notes for an account. I've been debugging some account balance issues lately and found this to be quite helpful. Here's what it looks like

If we add this, I'll open a PR to update docs on the website repo.

Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.

[ ] Yes
[x] No

new cli command - accounts:transactions

dguenther · 2022-03-12T03:33:16Z

Going to assign this to @NullSoldier because I think he wanted to give it a review next week before it merges. This looks useful!

QustomQure · 2022-03-30T11:36:42Z

I think that this command must be able to work with both accountName and account address whichever type was written for better usability.

wd021 · 2022-03-30T11:56:22Z

for consistency, I'm using the same format as accounts:balance, which is accountName. We can update to take both, although it is one quick step to get names from accounts:list

Allmaaz · 2022-04-12T18:42:17Z

Very good project

wd021 · 2022-05-19T07:46:29Z

@dguenther @NullSoldier still interested in merging this?

If so, i can update the code for latest version.. lmk!

dguenther · 2022-05-19T15:28:04Z

I'd still like to merge this, I think it's definitely useful especially in the latest testnet phase. Nobody on our side has been directly involved in reviewing + merging it, so sorry for the delay! I'd like to get one other person than myself to take a look at it, then it should be good to go, if you don't mind updating the code to fix conflicts.

rohanjadvani · 2022-05-19T15:47:48Z

ironfish/src/account/accounts.ts

+            memo: decryptedNote.memo().replace(/\x00/g, ''),
+          })
+
+          continue


Is this continue needed?

nope, removed.

xorinox · 2022-05-20T21:01:59Z

ironfish-cli/src/commands/accounts/transactions.test.ts

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
+import { expect as expectCli, test } from '@oclif/test'
+import { GetTransactionsResponse } from 'ironfish'


This import didn't work for me, I had to change the path to ../../../../ironfish/src/rpc/routes/transactions

yup, this needs updating for the latest version. I just pushed an update.

…ions-command

wd021 · 2022-05-23T14:45:45Z

@dguenther updated for the latest version, can review now.

mat-if · 2022-05-26T19:41:24Z

Hey @wd021, I'm starting to dig into this PR, sorry for the delay. I wanted to clarify which direction to look at this from.

What's the intended goal of this CLI command? I think there's a few different things technically happening here so I wanted to make sure we're on the same page on expectations of what questions this command should answer.

For example, are you expecting this to show a complete list of all transactions this account has participated in? A complete list of notes, spent and unspent? A list of transactions containing unspent notes? etc.

Looking at this right now, it feels a little bit awkward in that it is kind of a view into transactions, and also kind of a view into notes, and I think we can improve that together

wd021 · 2022-05-27T11:03:47Z

@mat-if no worries.

The motivation for adding this command was to help debug account balances & faucet errors during phase 1. I wanted an easy way to output the spent notes associated with a given account.

I agree that the wording is probably unclear. Would also be interested in expanding the scope of this into filtering by txs/notes/unspent notes.

mat-if · 2022-06-02T17:21:02Z

Ok, so I've thought about this a bit now. As I've mentioned, this feels a little bit like a view into both transactions and notes and feels a little confusing because of that.

I think this could go a couple different directions, let me know what you think.

This focuses on transactions, and stays as accounts:transactions.
This would summarize things like the value of notes, so instead of showing individual notes, you could just show the net value of the transactions (-10 $IRON, 5 $IRON), etc. Maybe show the number of spends and outputs as well. Jason suggested possibly showing the status of the transaction but that might be a lot more work, I haven't looked, so feel free to not do it here 😂. That can easily be a future improvement.
This focuses on notes, and becomes accounts:notes
This would instead show similar to how it is now, but without things like tx fee, since that's a transaction level concept and is unclear looking at the current table. If I didn't know better, I might think that each note is accruing a separate transaction fee. I would expect this to be mostly for debugging, whereas the other 2 also have use outside of debugging by providing better visibility that we currently lack.
Another interesting idea that I think could be useful alongside accounts:transactions would be like accounts:transaction <hash> to show the note-level breakdown of a specific transaction.

I think all 3 are very useful on their own, but to be clear, I don't expect you to implement all 3 in this PR. I think one is sufficient, and we can iterate from there. Let me know what you think!

If you want to continue on one of these, great! If not, let me know and I can take over the PR if you prefer.

wd021 · 2022-06-02T19:24:25Z

yea, 1 and 2 were exactly where my mind was at. let's have 2 separate methods, accounts:transactions and accounts:notes. And also agree that 3 could be useful too!

i don't mind taking a crack at all 3 in this PR, will have some time later in the week 👨‍💻

wd021 · 2022-06-06T10:12:46Z

@mat-if PR updated. here's the latest👇

accounts:notes - note level breakdown for given account (optional -a flag)

accounts:transactions - transaction breakdown for given account or txhash (optional -a / -t flags)

mat-if

Thanks for doing this! A few comments, but overall this looks pretty good.

mat-if · 2022-06-09T21:23:17Z

ironfish/src/account/accounts.ts

+    let transactionInfo = null
+    const transactionNotes = []
+
+    for (const transactionMapValue of this.transactionMap.values()) {


Since this function is for a specific transaction, instead of looping over this.transactionMap.values(), you can lookup on the hash directly: const transactionMapValue = this.transactionMap.get(Buffer.from(hash, 'hex'))

oh right, good catch.

mat-if · 2022-06-09T21:35:31Z

ironfish/src/account/accounts.ts

@@ -603,6 +603,46 @@ export class Accounts {
    this.scan = null
  }

+  getNotesFor(account: Account): {


What do you think about renaming this to getNotes to be more inline with the other function names in here?

Suggested change

getNotesFor(account: Account): {

getNotes(account: Account): {

makes sense 👌

mat-if · 2022-06-09T21:36:21Z

ironfish/src/account/accounts.ts

@@ -921,6 +961,125 @@ export class Accounts {
    await this.scanTransactions()
  }

+  getTransactionsFor(account: Account): {


Like above, what do you think about getTransactions

Suggested change

getTransactionsFor(account: Account): {

getTransactions(account: Account): {

makes sense 👌

mat-if · 2022-06-09T21:54:42Z

ironfish/src/account/accounts.ts

+        }
+      }
+
+      if (transactionCreator) {


If I'm understanding this correctly, this check makes it so we only add transactions if we were a spender of a note. I would expect to also see transactions in which I only received IRON as well. Or more specifically, I would expect to see a transaction in which I have any decryptable notes.

To show what I mean, I set up a test where this account has exactly 1 note, which was IRON I sent from a different account. You'll see the note, and it references a transaction, but when I do accounts:transactions it doesn't show anything

right. I updated this method to include notes where one is the receiver of iron in a transaction. but i also think its valuable to be able to detect the transactions created by the account, so there's now an extra creator column for the table.

mat-if · 2022-06-09T22:05:02Z

ironfish-cli/src/commands/accounts/transactions.ts

+
+    if (transactionInfo !== null) {
+      this.log(
+        `Transaction: ${transactionHash}, Status: ${transactionInfo.status}, Miner Fee: ${


What do you think about making each of these on a new line? I think it makes it a little easier to parse the info.

Transaction: fdaf29274587c8f58ac6214f42642fd610a222cea8e5e39ff4e11d58d379c194 Status: completed Miner Fee: x Fee ($ORE): 1 Spends: 1

updated to this

mat-if · 2022-06-09T22:22:18Z

ironfish/src/account/accounts.ts

+          }
+        }
+
+        if (transactionCreator) {


Same thing here about the transactionCreator thing, not able to look up a transaction that a note references if I only received in that transaction

yup, removed this.

wd021 · 2022-06-10T21:26:52Z

@mat-if

went through review changes and updated
added extra creator column for account:transactions
merge latest staging to resolve conflicts

mat-if

Thanks!

…ron-fish#1086) * new cli command - accounts:transactions new cli command - accounts:transactions * fix linting * add assertHasAccount() in getTransactionNotes * use variable 'account' instead of 'accountName' * update code to latest version * fix linting * refactor: convert accounts:transactions -> accounts:notes * refactor: getNotes type change * feat: new accounts:transactions command * fix: linting on test files * make review changes * fix test fail * fix lint

new cli command - accounts:transactions

b8bb56f

new cli command - accounts:transactions

wd021 requested a review from a team as a code owner February 28, 2022 14:07

wd021 added 2 commits February 28, 2022 14:13

fix linting

07f1d7b

add assertHasAccount() in getTransactionNotes

90be28f

wd021 mentioned this pull request Mar 5, 2022

improving accounts command #1089

Merged

use variable 'account' instead of 'accountName'

b081ceb

dguenther assigned NullSoldier Mar 12, 2022

rohanjadvani reviewed May 19, 2022

View reviewed changes

xorinox reviewed May 20, 2022

View reviewed changes

wd021 added 3 commits May 23, 2022 13:17

Merge remote-tracking branch 'upstream/staging' into account-transact…

6e3fae9

…ions-command

update code to latest version

001240f

fix linting

3c8b3fc

mat-if assigned mat-if and unassigned NullSoldier Jun 2, 2022

wd021 added 4 commits June 5, 2022 16:13

refactor: convert accounts:transactions -> accounts:notes

a1c037f

refactor: getNotes type change

30be84e

feat: new accounts:transactions command

7f33281

fix: linting on test files

ab81c43

wd021 changed the title ~~feat: new cli command accounts:transactions~~ feat: new cli commands accounts:notes and accounts:transactions Jun 6, 2022

mat-if requested changes Jun 9, 2022

View reviewed changes

wd021 added 4 commits June 10, 2022 23:35

make review changes

bbc5d6a

Merge branch 'staging' into account-transactions-command

f7ef07a

fix test fail

df1e83f

fix lint

845b403

mat-if approved these changes Jun 15, 2022

View reviewed changes

mat-if merged commit 7441c21 into iron-fish:staging Jun 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new cli commands `accounts:notes` and `accounts:transactions` #1086

feat: new cli commands `accounts:notes` and `accounts:transactions` #1086

wd021 commented Feb 28, 2022

dguenther commented Mar 12, 2022

QustomQure commented Mar 30, 2022 •

edited

wd021 commented Mar 30, 2022

Allmaaz commented Apr 12, 2022

wd021 commented May 19, 2022

dguenther commented May 19, 2022

rohanjadvani May 19, 2022

wd021 May 23, 2022

xorinox May 20, 2022

wd021 May 23, 2022

wd021 commented May 23, 2022

mat-if commented May 26, 2022

wd021 commented May 27, 2022

mat-if commented Jun 2, 2022

wd021 commented Jun 2, 2022

wd021 commented Jun 6, 2022

mat-if left a comment

mat-if Jun 9, 2022

wd021 Jun 10, 2022

mat-if Jun 9, 2022

wd021 Jun 10, 2022

mat-if Jun 9, 2022

wd021 Jun 10, 2022

mat-if Jun 9, 2022

wd021 Jun 10, 2022

mat-if Jun 9, 2022

wd021 Jun 10, 2022 •

edited

mat-if Jun 9, 2022

wd021 Jun 10, 2022

wd021 commented Jun 10, 2022 •

edited

mat-if left a comment

	getNotesFor(account: Account): {
	getNotes(account: Account): {

	getTransactionsFor(account: Account): {
	getTransactions(account: Account): {

feat: new cli commands accounts:notes and accounts:transactions #1086

feat: new cli commands accounts:notes and accounts:transactions #1086

Conversation

wd021 commented Feb 28, 2022

dguenther commented Mar 12, 2022

QustomQure commented Mar 30, 2022 • edited

wd021 commented Mar 30, 2022

Allmaaz commented Apr 12, 2022

wd021 commented May 19, 2022

dguenther commented May 19, 2022

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

wd021 commented May 23, 2022

mat-if commented May 26, 2022

wd021 commented May 27, 2022

mat-if commented Jun 2, 2022

wd021 commented Jun 2, 2022

wd021 commented Jun 6, 2022

mat-if 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

wd021 Jun 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wd021 commented Jun 10, 2022 • edited

mat-if left a comment

Choose a reason for hiding this comment

feat: new cli commands `accounts:notes` and `accounts:transactions` #1086

feat: new cli commands `accounts:notes` and `accounts:transactions` #1086

QustomQure commented Mar 30, 2022 •

edited

wd021 Jun 10, 2022 •

edited

wd021 commented Jun 10, 2022 •

edited