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

Dev simple spend #129

Merged
merged 7 commits into from
Jul 27, 2023
Merged

Conversation

frier17
Copy link
Contributor

@frier17 frier17 commented Jul 22, 2023

Hi. I am proposing these changes to simple_spend with respect to issue #122 by @haysquareA. I have modified the simple_spend to take a preference parameter which can be used as a flag to set the transaction fee as low, medium, zero, or high in creating an unsigned transaction.
I also added update to the simple_spend documentation to reflect the preference variable.
Lastly, a test was added as SimpleSpendTX in the test_blockcypher.py script. This tests uses example from the blockcypher documentation with values set to fail due to insufficient funds in the address used.

Kindly review.

Thanks.

Copy link
Contributor

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @frier17. Looking good. One minor comment regarding defaulting to low fees.

@@ -1624,7 +1624,7 @@ def broadcast_signed_transaction(unsigned_tx, signatures, pubkeys, coin_symbol='


def simple_spend(from_privkey, to_address, to_satoshis, change_address=None,
privkey_is_compressed=True, min_confirmations=0, api_key=None, coin_symbol='btc'):
privkey_is_compressed=True, min_confirmations=0, api_key=None, coin_symbol='btc', preference: str = 'low'):
Copy link
Contributor

Choose a reason for hiding this comment

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

For a better UX I suggest that keep the fee high here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback @quentinlesceller. I plan making the needed changes. Also noticed a similar need for the function simple_spend_p2sh. Will attempt to make similar changes and add tests as well.

Thanks

@quentinlesceller
Copy link
Contributor

Oh just noticed the zero fee. Transaction with zero fees will be directly rejected by most transaction pools so I would remove the option.

@frier17
Copy link
Contributor Author

frier17 commented Jul 27, 2023

Thanks for the feedback @quentinlesceller. I have removed the zero option and updated the tests. Similar work was done on the simple_spend_p2sh function.

Copy link
Contributor

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

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

Amazing. Thank you for the PR! LGTM!

@quentinlesceller quentinlesceller merged commit dac3bc6 into blockcypher:master Jul 27, 2023
@frier17 frier17 deleted the dev_simple_spend branch July 31, 2023 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants