Re-added code to clean branch (JavaScript Desktop Wallet) #1964

AlexanderBuzz · 2023-06-20T11:58:40Z

No description provided.

AlexanderBuzz · 2023-06-20T11:59:34Z

@JST5000 @mDuo13 This is the "JavaScript Desktop Wallet" in a new branch without all those hiccups

AlexanderBuzz · 2023-06-20T11:59:51Z

@SocketSecurity ignore [email protected]

socket-security · 2023-06-20T11:59:55Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

socket-security · 2023-06-20T11:59:58Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
node-fetch	2.6.12	network	`+3`	493 kB	node-fetch-bot
async	3.2.4	None	`+0`	821 kB	hargasinski
toml	3.0.0	filesystem	`+0`	144 kB	binarymuse
electron	22.3.2	network, filesystem, shell, environment	`+50`	5.89 MB	electron-nightly

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md

…ipt.md Co-authored-by: Jackson Mills <[email protected]>

AlexanderBuzz · 2023-07-03T18:05:10Z

I'm currently reworking the steps to "code along" style, testing the tutorial myself to make sure everything works. I will drop you a note when I'm done which will also take care of the issues that are still open.

AlexanderBuzz · 2023-07-04T17:45:06Z

@JST5000 1-7 is in, I've even played it through myself, repo here: https://github.com/AlexanderBuzz/xrpl-dev-desktop-wallet. Contains one commit per step, so it should work just from the explanation without jumping to external files.

AlexanderBuzz · 2023-07-05T15:37:40Z

@JST5000 I forgot to push, now all steps are in.

JST5000 · 2023-07-07T00:53:01Z

Hoping to review tomorrow :)

JST5000 · 2023-08-11T23:24:38Z

Three remaining comments, but seems solid. I reached out to my teammate for next steps - once you've resolved those issues I'm happy to approve though!

AlexanderBuzz · 2023-08-14T15:38:51Z

@JST5000 Ok, everything done, I left the bootstrap issue open for review.

JST5000 · 2023-08-15T00:08:41Z

One last comment from me - There's an error check which would be super helpful to avoid the app performing very oddly when entering the wrong password right here: https://github.com/XRPLF/xrpl-dev-portal/pull/1964/files/183bed65b131bc7c12bca00bb414a77822eb8742#r1283698777

AlexanderBuzz · 2023-08-15T16:44:15Z

@JST5000 I've added that check, as simple as possible, in order to bloat this mote than necessary. Next thing to add would be a state manager :)

mDuo13

I was making good progress through the tutorial, fixing a couple typos and suggesting some non-critical improvements to the wording, until I got to step 4, where I found a big problem.

This tutorial, as written, creates a wallet that is vulnerable to the partial payments exploit because it uses the transaction's Amount field rather than delivered_amount. That needs to change. I also noticed some other discrepancies that stopped me at step 4.

It feels really close, but I definitely can't approve this until the vulnerability is fixed.

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md

content/_code-samples/build-a-wallet/desktop-js/library/4_helpers.js

mDuo13 · 2023-08-18T23:10:16Z

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md

+  // Step 4 code additions - start
+  const transactions = prepareTxData([{tx: transaction.transaction}])
+  appWindow.webContents.send('update-transaction-data', transactions)
+  // Step 4 code additions - end


This part doesn't seem to be enough on its own. Comparing mine to the version of index.js in 4-tx-history/, I'm missing the "Initial Transaction Request" section (the steps as called out in the tutorial never have you add it) and I'm not sure if that or something else is why I was getting a ReferenceError: transaction is not defined error at this stage, and the transaction table not populating.

It vanished from the tutorial somehow, I've re-added it

I don't think these changes got pushed to the branch—it looks like they're still missing here.

mDuo13 · 2023-08-19T00:58:22Z

P.S. the other JS wallet tutorial has a similar problem, although it was less obvious because it only says "Amount" rather than "Amount delivered". I've opened #2075 to get that one fixed ASAP as well.

AlexanderBuzz · 2023-09-06T13:51:19Z

@mDuo13 I'll fix that when in back from APEX.

AlexanderBuzz · 2023-09-15T07:23:50Z

Continuing today, but we should see that we get a lid on this now. With the squashed branch from a couple of weeks ago we're at over 350 commits. Don't know why this bounty is expected to provide 10x the value it is valued at.

…ipt.md Co-authored-by: Rome Reginelli <[email protected]>

- added custom token to displayable amount

mDuo13

Good stuff! I still ran into a couple problems when running through the tutorial, but at this point it's close enough that I'm happy to resolve them myself as I merge this. Thanks so much for your efforts! For what it's worth, I've put in a word to get you a bonus since the level of effort you put into this bounty was above and beyond.

mDuo13 · 2023-09-22T21:33:24Z

content/_code-samples/build-a-wallet/desktop-js/3-account/view/renderer.js

+// Step 3 - Section start - this remains as it is, the rest is new
+const ledgerIndexEl = document.getElementById('ledger-index')
+const ledgerHashEl = document.getElementById('ledger-hash')
+const ledgerCloseTimeEl = document.getElementById('ledger-close-time')
+
+window.electronAPI.onUpdateLedgerData((_event, ledger) => {
+    ledgerIndexEl.innerText = ledger.ledgerIndex
+    ledgerHashEl.innerText = ledger.ledgerHash
+    ledgerCloseTimeEl.innerText = ledger.ledgerCloseTime
+})
+// Step 3 - Section end


This doesn't actually remain "as it is" from the previous step, which uses snake_case fields like ledger.ledger_index instead of camelCase fields like the ledger.ledgerIndex fields now being passed in from prepareLedgerData(...).

Probably the easiest fix is to tell the user to replace the whole file.

Suggested change

// Step 3 - Section start - this remains as it is, the rest is new

const ledgerIndexEl = document.getElementById('ledger-index')

const ledgerHashEl = document.getElementById('ledger-hash')

const ledgerCloseTimeEl = document.getElementById('ledger-close-time')

window.electronAPI.onUpdateLedgerData((_event, ledger) => {

ledgerIndexEl.innerText = ledger.ledgerIndex

ledgerHashEl.innerText = ledger.ledgerHash

ledgerCloseTimeEl.innerText = ledger.ledgerCloseTime

})

// Step 3 - Section end

const ledgerIndexEl = document.getElementById('ledger-index')

const ledgerHashEl = document.getElementById('ledger-hash')

const ledgerCloseTimeEl = document.getElementById('ledger-close-time')

window.electronAPI.onUpdateLedgerData((_event, ledger) => {

ledgerIndexEl.innerText = ledger.ledgerIndex

ledgerHashEl.innerText = ledger.ledgerHash

ledgerCloseTimeEl.innerText = ledger.ledgerCloseTime

})

mDuo13 · 2023-09-22T21:38:48Z

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md

+        <form method="dialog">
+```
+
+The table here will be filled dynamically with the accounts transactions.


Fix typo

Suggested change

The table here will be filled dynamically with the accounts transactions.

The table here will be filled dynamically with the account's transactions.

mDuo13 · 2023-09-22T22:22:37Z

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md

+  // Step 4 code additions - start
+  const transactions = prepareTxData([{tx: transaction.transaction}])
+  appWindow.webContents.send('update-transaction-data', transactions)
+  // Step 4 code additions - end


I don't think these changes got pushed to the branch—it looks like they're still missing here.

mDuo13 · 2023-09-22T22:40:14Z

content/_code-samples/build-a-wallet/desktop-js/library/4_helpers.js

+        confirmed: transaction.tx.date,
+        type: transaction.tx.TransactionType,
+        from: transaction.tx.Account,
+        to: transaction.tx.Destination,


Since this function can get passed any type of transaction, we need to handle transactions that don't have a Destination field. This will return the string - if the field is undefined:

Suggested change

to: transaction.tx.Destination,

to: transaction.tx.Destination ?? "-",

mDuo13 · 2023-09-22T22:48:19Z

content/_code-samples/build-a-wallet/desktop-js/library/4_helpers.js

+}
+
+const getDisplayableAmount = (rawAmount) => {
+    if (rawAmount === 'unavailable') {


Similar to above, not all transactions have a delivered_amount field. (There are ones that don't deliver value, like AccountSet for example, and ones that don't have it implemented like EscrowFinish.)

The browser wallet handles this in the code that calls this function, but for this tutorial I think it's easier to handle it here by adding a case for undefined:

Suggested change

if (rawAmount === 'unavailable') {

if (rawAmount === undefined) {

// Not all transaction types have delivered_amount

return "-"

} else if (rawAmount === 'unavailable') {

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md

Co-authored-by: Jackson Mills <[email protected]>

Re-added code to clean branch

b96d3a3

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JST5000 reviewed Jun 22, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

mDuo13 mentioned this pull request Jun 23, 2023

Bounties 0077 js samples 2 Create-a-Wallet #1734

Closed

AlexanderBuzz and others added 8 commits July 1, 2023 16:43

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

84d9a3c

…ipt.md Co-authored-by: Jackson Mills <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

5d17e9e

…ipt.md Co-authored-by: Jackson Mills <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

250a9c7

…ipt.md Co-authored-by: Jackson Mills <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

3853b76

…ipt.md Co-authored-by: Jackson Mills <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

b9eabf2

…ipt.md Co-authored-by: Jackson Mills <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

2fd5bb4

…ipt.md Co-authored-by: Jackson Mills <[email protected]>

Restructured Step 0 + 1 to "code along" style

3d9a3ca

Restructured Step 0 + 1 to "code along" style

bad6ce5

Restructured Step 2 - 7 to "code along" style

b46e8a6

Restructured Step 8 to "code along" style

f19b4b6

AlexanderBuzz added 3 commits August 14, 2023 16:21

Clarified how to include bootstrap

f57a654

Directly linked bootrap includes

1c5737f

Clarified Step 1 instructions

66908d0

mDuo13 self-assigned this Aug 14, 2023

Clarified Step 1 instructions

742e892

mDuo13 requested changes Aug 18, 2023

View reviewed changes

mDuo13 assigned AlexanderBuzz and unassigned mDuo13 Aug 30, 2023

AlexanderBuzz and others added 10 commits September 18, 2023 20:08

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

2b7716f

…ipt.md Co-authored-by: Rome Reginelli <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

da230d4

…ipt.md Co-authored-by: Rome Reginelli <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

112c404

…ipt.md Co-authored-by: Rome Reginelli <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

59462b1

…ipt.md Co-authored-by: Rome Reginelli <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

58e410b

…ipt.md Co-authored-by: Rome Reginelli <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

ac9d9c7

…ipt.md Co-authored-by: Rome Reginelli <[email protected]>

Update content/tutorials/build-apps/build-a-desktop-wallet-in-javascr…

2f8f7d9

…ipt.md Co-authored-by: Rome Reginelli <[email protected]>

- Fixed partial payment exploit

2fcab18

- added custom token to displayable amount

- Fixed snippet markers in tutorial for Step 3

ff65431

- Re-adde missing Snippet to step 4 intsructions

03e4596

AlexanderBuzz requested a review from mDuo13 September 22, 2023 13:05

mDuo13 approved these changes Sep 23, 2023

View reviewed changes

mDuo13 reviewed Sep 23, 2023

View reviewed changes

content/tutorials/build-apps/build-a-desktop-wallet-in-javascript.md Outdated Show resolved Hide resolved

JS Desktop Wallet: edits per review

3a8da51

Co-authored-by: Jackson Mills <[email protected]>

mDuo13 merged commit 3a8da51 into XRPLF:master Sep 23, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-added code to clean branch (JavaScript Desktop Wallet) #1964

Re-added code to clean branch (JavaScript Desktop Wallet) #1964

AlexanderBuzz commented Jun 20, 2023

AlexanderBuzz commented Jun 20, 2023

AlexanderBuzz commented Jun 20, 2023 •

edited

Loading

socket-security bot commented Jun 20, 2023 •

edited

Loading

socket-security bot commented Jun 20, 2023 •

edited

Loading

AlexanderBuzz commented Jul 3, 2023

AlexanderBuzz commented Jul 4, 2023

AlexanderBuzz commented Jul 5, 2023

JST5000 commented Jul 7, 2023

JST5000 commented Aug 11, 2023

AlexanderBuzz commented Aug 14, 2023

JST5000 commented Aug 15, 2023

AlexanderBuzz commented Aug 15, 2023 •

edited

Loading

mDuo13 left a comment

mDuo13 Aug 18, 2023

AlexanderBuzz Sep 22, 2023

mDuo13 Sep 22, 2023

mDuo13 commented Aug 19, 2023

AlexanderBuzz commented Sep 6, 2023

AlexanderBuzz commented Sep 15, 2023

mDuo13 left a comment

mDuo13 Sep 22, 2023

mDuo13 Sep 22, 2023

mDuo13 Sep 22, 2023

mDuo13 Sep 22, 2023

mDuo13 Sep 22, 2023

	The table here will be filled dynamically with the accounts transactions.
	The table here will be filled dynamically with the account's transactions.

	to: transaction.tx.Destination,
	to: transaction.tx.Destination ?? "-",

-    if (rawAmount === 'unavailable') {
+    if (rawAmount === undefined) {
+        // Not all transaction types have delivered_amount
+        return "-"
+    } else if (rawAmount === 'unavailable') {

Re-added code to clean branch (JavaScript Desktop Wallet) #1964

Re-added code to clean branch (JavaScript Desktop Wallet) #1964

Conversation

AlexanderBuzz commented Jun 20, 2023

AlexanderBuzz commented Jun 20, 2023

AlexanderBuzz commented Jun 20, 2023 • edited Loading

socket-security bot commented Jun 20, 2023 • edited Loading

socket-security bot commented Jun 20, 2023 • edited Loading

AlexanderBuzz commented Jul 3, 2023

AlexanderBuzz commented Jul 4, 2023

AlexanderBuzz commented Jul 5, 2023

JST5000 commented Jul 7, 2023

JST5000 commented Aug 11, 2023

AlexanderBuzz commented Aug 14, 2023

JST5000 commented Aug 15, 2023

AlexanderBuzz commented Aug 15, 2023 • edited Loading

mDuo13 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

mDuo13 commented Aug 19, 2023

AlexanderBuzz commented Sep 6, 2023

AlexanderBuzz commented Sep 15, 2023

mDuo13 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

AlexanderBuzz commented Jun 20, 2023 •

edited

Loading

socket-security bot commented Jun 20, 2023 •

edited

Loading

socket-security bot commented Jun 20, 2023 •

edited

Loading

AlexanderBuzz commented Aug 15, 2023 •

edited

Loading