wallet: drop change to fee if amount is less than dust

Bitshala-Incubator / silent-pay

a TS library for silent payments

MIT License

12 stars 13 forks source link

wallet: drop change to fee if amount is less than dust #32

Closed Ayush170-Future closed 9 months ago

Ayush170-Future commented 9 months ago

This commit refactors the CoinSelector's select() method to properly handle the calculation of change cost. The change amount is now checked against the cost of change, and if the change is less than the cost, the cost is added to the transaction fee. Additionally, it adds a unit test for the CoinSelector.

Fixes: Issue #26

Ayush170-Future commented 9 months ago

Can we add a test

Yes. I'll fix the lint and add a test in the next force push.

Ayush170-Future commented 9 months ago

Force pushed:

Hopefully fixes Lint
Adds a unit test for CoinSelector

Ayush170-Future commented 9 months ago

Force pushed 3c1983e addressing requested changes.

Ayush170-Future commented 9 months ago

Force pushed a9cf64d refactoring the unit test and creating random script for the test transaction output.

Ayush170-Future commented 9 months ago

Force pushed 0e8a816, which changed the random data into constant values.

I agree with @emjshrx that using random values in unit testing could lead to non-deterministic behavior. Therefore, I ran my code and recorded all the values it produced. I then hard-coded this static data to ensure deterministic results every time.
I also covered both cases: when the change > dust and when the change <= dust.
Fixed the nits

Ayush170-Future commented 9 months ago

Force pushed ed44b85

Used it.each()
Added comments for justifying my calculations
Replaced less than dust values in the coins with bigger values (more than 1000)
Rebased on the main

Ayush170-Future commented 9 months ago

Force pushed 4589fff addressing reviews and rebasing to the main.

Ayush170-Future commented 9 months ago

Done!!

Addressed the nit

theanmolsharma commented 9 months ago

@Ayush170-Future you need to sign the commits so that we can merge. I've enabled branch protection rules that only allow signed commits to be merged in main. It's a good FOSS practice to sign commits. If you don't have a GPG setup now, I can sign the commits for this PR, but all commits in the future must be signed.

Some helpful links: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

Ayush170-Future commented 9 months ago

@Ayush170-Future you need to sign the commits so that we can merge. I've enabled branch protection rules that only allow signed commits to be merged in main. It's a good FOSS practice to sign commits. If you don't have a GPG setup now, I can sign the commits for this PR, but all commits in the future must be signed.

Yes, please sign the commits in this PR, and thanks for letting me know, I'll set it up and sign my commits from now on.