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

Create bitcoin_utils.py to make bitcoin payments available to agents #750

Closed
wants to merge 5 commits into from

Conversation

dbscooper
Copy link

Background

Changes

Documentation

Test Plan

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thouroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes

Copy link
Contributor

@richbeales richbeales left a comment

Choose a reason for hiding this comment

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

Interesting idea, but this isn't referred to in the prompt, so would never (I think) actually get used.
Also, I'm assuming 'bitcoin' is a pip package, and wouldn't be part of the standard python distro, so would need adding to requirements.txt

Also, this is where the plugin PR comes in, as arguably not everyone wants/needs bitcoin support.

nponeccop
nponeccop previously approved these changes Apr 11, 2023
Copy link
Contributor

@nponeccop nponeccop left a comment

Choose a reason for hiding this comment

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

Even if it's useless it's a good self-contained module to use later on.

@nponeccop nponeccop mentioned this pull request Apr 11, 2023
1 task
@benthecarman
Copy link

Shouldnt this also need a get address function?

if not unspent:
raise Exception('No available UTXOs for the sender address.')

unspent.sort(key=lambda x: -x['amount'])

Choose a reason for hiding this comment

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

you should randomize the list, if you sort by amount you'll either grind your utxos to dust or pay way too much fees

This shuffles the UTXOs before selecting them for use in the transaction, helping to avoid grinding them to dust or overpaying fees
nponeccop
nponeccop previously approved these changes Apr 12, 2023
@nponeccop
Copy link
Contributor

It's not a good idea to keep adding things to a reviewed commit without a reviewer's request. Create another commit instead and say that it's dependent on this one. @Torantulino, @richbeales , @p-i- ?

@dbscooper
Copy link
Author

Ok, sorry, new to this collaboration business!

bitcoin_utils.py Outdated
SelectParams(network)
rpc = Proxy()

# Rest of the send_bitcoin function implementation

Choose a reason for hiding this comment

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

think you forgot to do this

@richbeales richbeales added the needs discussion To be discussed among maintainers label Apr 13, 2023
@talvasconcelos
Copy link

Just my two sats on this, but i thing Bitcoin's Lightning Network would more appropriate for this.

@Pwuts
Copy link
Member

Pwuts commented Apr 22, 2023

This is out of scope for the core project (as of #757) but could be a plugin!

@Pwuts Pwuts closed this Apr 22, 2023
@Pwuts Pwuts added the potential plugin This may fit better into our plugin system. label Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion To be discussed among maintainers potential plugin This may fit better into our plugin system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants