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.

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

Tidy up transaction.js #88

Merged
merged 8 commits into from
Mar 1, 2023
Merged

Tidy up transaction.js #88

merged 8 commits into from
Mar 1, 2023

Conversation

Duddino
Copy link
Member

@Duddino Duddino commented Feb 23, 2023

Abstract

Tidy up transaction.js

What does this PR address?

This PR cleans up transaction.js halving the lines of copy-pasted code and removing unused functions

How does this benefit users?

The code is easier to audit and edit

@JSKitty JSKitty added Documentation Improvements or additions to documentation Enhancement New feature or request labels Feb 24, 2023
panleone
panleone previously approved these changes Feb 27, 2023
Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

tACK 592e017:
normal tx delegation undelegation and MN creation working as expected

JSKitty
JSKitty previously approved these changes Feb 27, 2023
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

tACK, tested with over 1000 UTXOs in many different transaction styles (Send - Delegate - Undelegate) 🎉

Extremely clean - great to finally have our last messy part of 'coin control' centralised in to a single transaction function finally, massive code reduction there alone.

Great job! 🚀 🔥

Co-authored-by: JSKitty <jskitty@protonmail.com>
@Duddino Duddino dismissed stale reviews from JSKitty and panleone via 8e6bfd5 February 28, 2023 14:26
@Duddino Duddino mentioned this pull request Feb 28, 2023
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

re-tACK 8e6bfd5 - Perfect! 🎉

Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

(re) tACK 8e6bfd5

@JSKitty JSKitty merged commit 2bca2f3 into PIVX-Labs:master Mar 1, 2023
Duddino added a commit to Duddino/MyPIVXWallet that referenced this pull request Mar 9, 2023
* Rewritten delegate, undelegate, sendTx

* Now they work

* Ran prettier

* Added error checking

* Restored wallet.js since it will be fixed in PIVX-Labs#87

* Added jsdoc comments

* Apply suggestions from code review

Co-authored-by: JSKitty <jskitty@protonmail.com>

---------

Co-authored-by: JSKitty <jskitty@protonmail.com>
Duddino added a commit to Duddino/MyPIVXWallet that referenced this pull request Mar 9, 2023
* Rewritten delegate, undelegate, sendTx

* Now they work

* Ran prettier

* Added error checking

* Restored wallet.js since it will be fixed in PIVX-Labs#87

* Added jsdoc comments

* Apply suggestions from code review

Co-authored-by: JSKitty <jskitty@protonmail.com>

---------

Co-authored-by: JSKitty <jskitty@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to documentation Enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

3 participants