Updates to the project made by Dov #1

danimoh · 2025-01-28T20:41:18Z

These changes were made by Dov and had been merged from https://github.com/DovAzencot/cashlink-generator into temporary branch update-dov and shall upon review be merged into branch update which serves as the development branch until the update is concluded.

Improvements:

- Complete TypeScript rewrite with enhanced type safety
- Modern environment configuration handling
- Updated dependencies for latest Node.js compatibility
- Nimiq 2.0 (Albatross PoS) support
- Improved development experience with proper typing
- Enhanced QR code generation capabilities

See changes to README included in this PR.

Cherry pick project update f8b73aa ("first commit") by DovAzencot from separate remote [email protected]:DovAzencot/cashlink-generator.git, which is a fork of this repository, which was not initialized as a fork though, i.e. it has an unrelated commit history. It could have been merged via --allow-unrelated-histories instead of a cherry- pick, but a cherry-pick avoids the unrelated commit history to better preserve git blame annotations.

danimoh

Hi @DovAzencot,

thanks for the extensive update. The migration to Nimiq PoS seems to work nicely and I welcome the addition of documentation and the modernization of code to typescript and es6 modules. Overall and on a high level, I like this update. However, on close review there are of course also details I like less and which I want to give further feedback on:

It would have been nice to submit this as a PR to the existing project, instead of as a separate project. That would have simplified reviewing against the existing code, leaving feedback and eventually merging the changes into the main project. I now cherry-picked your changes onto this project and opened this pull request to achieve basically that.
This PR / update includes too much. It would have been better to first focus on the task at hand and then take it from there. That would also have simplified the review.
The whole thing is a single commit. Better would have been well encapsulated commits per change or feature with well written commit messages.
Assuming the copyright of the entire project is not cool.
Apart from the good changes, there are also unnecessary, unrelated or bogus changes, and not all of those are even improvements. Some of those changes introduce bugs, add code that provides no benefit, change the coding style deviating from what our usual coding style is, or are bogus changes like changing the order of methods in a file, which further complicates diffing and reviewing.
Many useful code comments have been removed for no obvious reason. Those are comments that the people that originally wrote the code put some thought into and added for a reason. I know, because for this project that's basically me :D

I did a full, detailed review of this PR. Further feedback and issues on relevant code snippets are given in the comments below. Please take a look at those.

You can continue working on this, if you want, but it's not expected. I will probably take this on at some point, but for now finalizing this PR is not the highest priority, and the functionality is basically already there for if Cashlinks need to be generated now.

danimoh · 2025-01-28T20:43:27Z

.env.example

+# Salt for cashlink generation (base64 encoded)
+SALT=your-base64-encoded-salt-here


Should add a note that this salt must be kept absolutely secret.

danimoh · 2025-01-28T20:45:38Z

LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2025 Dov Azencot


Assuming copyright on the whole thing is maybe a bit much.

I did mention you in the readme, obviously forgot to add you here, sorry! <3

danimoh · 2025-01-28T20:49:52Z

package.json

@@ -1,15 +1,27 @@
 {
-  "name": "cashlink-generator",
+  "name": "cashlink",


This is not a Cashlink library, so I think cashlink-generator was the more suitable name.

danimoh · 2025-01-28T20:52:00Z

package.json

+    "qr": "NODE_OPTIONS='--no-deprecation' tsx src/render-qr-codes.ts"
+  },
+  "keywords": [],
+  "author": "Dov Azencot",


This as well should stay Nimiq, nimiq.com.

danimoh · 2025-01-28T20:52:38Z

package.json

  "license": "MIT",
+  "packageManager": "[email protected]+sha512.22721b3a11f81661ae1ec68ce1a7b879425a1ca5b991c975b074ac220b187ce56c708fe5db69f4c962c989452eee76c82877f4ee80f474cebd61ee13461b6228",


Haha, not a big fan of pnpm but alright, alright ;)
(Maybe I'll keept it. Maybe.)

danimoh · 2025-01-31T22:43:09Z

src/index.ts

+    catch (error) {
+        console.error('Error:', error instanceof Error ? error.message : 'Unknown error');
+        process.exit(1);
    }


While generally thinking about error handling is a good idea, I don't think that the try ... catch here is beneficial. Unhandled exceptions terminate the node process anyway and provide more information like the stack trace.
The same is arguably true for several of the other introduced try ... catch blocks, which additionally lead to a lot of indentation changes.

danimoh · 2025-01-31T22:49:51Z

src/index.ts

+// Execute and handle unhandled rejections
+main().catch(error => {
+    console.error('Unhandled error:', error);
+    process.exit(1);
+});


The main method does not throw any errors, as it handles them itself. But even if it would, arguably it is better to also not handle them here, see above.

danimoh · 2025-02-02T09:59:15Z

test/index.ts

+        /**
+         * Test 5: Transaction Receipt
+         * Verifies transaction receipt retrieval functionality
+         */
+        const txHash = 'd15bd926ad0c0e52346badf29cc128a26369d13a45498ad5e6a122f986054132';
+        const transaction = await client.getTransactionReceipt(txHash);
+        console.log(`Transaction ${txHash} was sent on block: `,transaction.blockNumber);


getTransactionReceipt is actually not used by the project, so it can be removed from the tests and rpc-client. Instead, getTransactionsByAddress should be tested.

danimoh · 2025-02-02T10:50:13Z

src/index.ts

+ * - Generate QR codes and coin images
+ * - Create usage statistics
+ * 
+ * The tool supports both interactive CLI usage and programmatic integration.


Programmatic integration (reading from command line parameters) is not implemented.

danimoh · 2025-02-02T10:54:30Z

src/cashlink-statistics-handler.ts

-            let wasFunded = false;
-            let wasUserClaimed = false;
-            let wasReclaimed = false;
+        console.log(cashlinkUserFriendlyAddress)


This spams the logs quite a bit.

danimoh force-pushed the update-dov branch from 89855c0 to e021428 Compare January 29, 2025 11:22

danimoh commented Feb 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to the project made by Dov #1

Updates to the project made by Dov #1

danimoh commented Jan 28, 2025

danimoh left a comment

danimoh Jan 28, 2025

danimoh Jan 28, 2025

DovAzencot Feb 3, 2025

danimoh Jan 28, 2025

danimoh Jan 28, 2025

danimoh Jan 28, 2025

danimoh Jan 31, 2025

danimoh Jan 31, 2025

danimoh Feb 2, 2025

danimoh Feb 2, 2025

danimoh Feb 2, 2025

		# Salt for cashlink generation (base64 encoded)
		SALT=your-base64-encoded-salt-here

		"license": "MIT",
		"packageManager": "[email protected]+sha512.22721b3a11f81661ae1ec68ce1a7b879425a1ca5b991c975b074ac220b187ce56c708fe5db69f4c962c989452eee76c82877f4ee80f474cebd61ee13461b6228",

Updates to the project made by Dov #1

Are you sure you want to change the base?

Updates to the project made by Dov #1

Conversation

danimoh commented Jan 28, 2025

danimoh 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment