Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(ironfish): add spends to wallet/getTransaction #3835

Merged
merged 4 commits into from
Apr 24, 2023
Merged

feat(ironfish): add spends to wallet/getTransaction #3835

merged 4 commits into from
Apr 24, 2023

Conversation

holahula
Copy link
Contributor

@holahula holahula commented Apr 24, 2023 •
edited
Loading

Summary

Follow-up to #3834
Linear task https://linear.app/if-labs/issue/IFL-723/add-spend-to-rpc-walletgettransaction

Closes IFL-733

Testing Plan

➜  ironfish git:(holahula/feat/wallet-get-transaction-vin) ✗ fish wallet:transaction 4b6ca31c20360291fd81d8f15ac2677236db0c6993fc2e95a0ae0acab3178eb2 -d ~/.ironfish-simulator/node-1c55
yarn run v1.22.19
$ yarn build && yarn start:js wallet:transaction 4b6ca31c20360291fd81d8f15ac2677236db0c6993fc2e95a0ae0acab3178eb2 -d /Users/austino/.ironfish-simulator/node-1c55
$ tsc -b
$ cross-env OCLIF_TS_NODE=0 IRONFISH_DEBUG=1 node --expose-gc --inspect=:0 --inspect-publish-uid=http --enable-source-maps bin/run wallet:transaction 4b6ca31c20360291fd81d8f15ac2677236db0c6993fc2e95a0ae0acab3178eb2 -d /Users/austino/.ironfish-simulator/node-1c55
Transaction: 4b6ca31c20360291fd81d8f15ac2677236db0c6993fc2e95a0ae0acab3178eb2
Account: default
Status: confirmed
Type: receive
Timestamp: 4/24/2023 1:06:22 p.m. EDT
Fee: $IRON 1.00000000
Block Hash: 00002c250d9b7491387c08704433d71b3e20ad7db56f9c72a437e946bb0ad721
Block Sequence: 297
Notes Count: 2
Spends Count: 1
Mints Count: 0
Burns Count: 0
Sender: d6ab204e37848c7bfe8d2cd7b10d4cf0764cfe06758f6d2c149bafd71e7b613c

---Notes---

 Amount     Asset Name Asset Id                                                     Spent Memo         Owner Owner Address
 ────────── ────────── ──────────────────────────────────────────────────────────── ───── ──────────── ───── ────────────────────────────────────────────────────────────────
 1.00000000 $IRON      51f33a2f14f92735e562dc658a5639279ddca3d5079a6d1242b2a588a9c… x     default memo ✔     7883a985cb103a0827cd78f9e9602e64ea5739491e4530547ebf673f5281f0dc

---Spends---

 Size Nullifier                                                        Commitment
 ──── ──────────────────────────────────────────────────────────────── ────────────────────────────────────────────────────────────────
 406  4e8bbbd9dc23056dee5015f62e133852316c5f10697de9277ab8459a87abec70 fc9b3502574aaa46e6557f8726fc34450f3bc0ae784a004e46f0f91e362ddb63

---Asset Balance Deltas---

 Asset ID                                                         Balance Change
 ──────────────────────────────────────────────────────────────── ──────────────
 51f33a2f14f92735e562dc658a5639279ddca3d5079a6d1242b2a588a9cbf44c 100000000
✨  Done in 2.83s.

Documentation

Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference)? If yes, link a
related documentation pull request for the website.

[X] Yes

iron-fish/website#400

Breaking Change

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

[ ] Yes

@holahula holahula mentioned this pull request Apr 24, 2023
Merged
@holahula holahula marked this pull request as ready for review April 24, 2023 21:17
@holahula holahula requested a review from a team as a code owner April 24, 2023 21:17
@holahula holahula changed the title feat(ironfish): add spends to wallet/getTransaction feat(ironfish): add spends to wallet/getTransaction Apr 24, 2023
ygao76
ygao76 reviewed Apr 24, 2023
View reviewed changes
Copy link
Contributor

@ygao76 ygao76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, can you add unit test?

@holahula
Copy link
Contributor Author

Looks good, can you add unit test?

added

@holahula holahula closed this Apr 24, 2023
@holahula holahula reopened this Apr 24, 2023
@holahula holahula mentioned this pull request Apr 24, 2023
Merged
ygao76
ygao76 reviewed Apr 24, 2023
View reviewed changes
ironfish/src/rpc/routes/wallet/getTransaction.test.ts Outdated
expect(responseAccount).toMatch(account.name)

expect(responseTransaction.spends).toHaveLength(1)
expect(responseTransaction.notes).toHaveLength(2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you compare the length with transaction.notes.length

ygao76
ygao76 reviewed Apr 24, 2023
View reviewed changes
ironfish/src/rpc/routes/wallet/getTransaction.test.ts Outdated

expect(responseAccount).toMatch(account.name)

expect(responseTransaction.spends).toHaveLength(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you compare it with transaction.spends, and it would be nice if you can compare the each attributes in spends

ygao76
ygao76 approved these changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@ygao76 ygao76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left two comments, rest of the PR looks good. Approve it with the condition the comments are addressed.

@ygao76 ygao76 merged commit 1c2c600 into staging Apr 24, 2023
@ygao76 ygao76 deleted the holahula/feat/wallet-get-transaction-vin branch April 24, 2023 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ygao76 ygao76 ygao76 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@holahula @ygao76