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

Remove default for parameter --fee-rate for wallet send #1755

Merged
merged 8 commits into from
Feb 15, 2023

Conversation

veryordinally
Copy link
Collaborator

@veryordinally veryordinally commented Feb 15, 2023

Implement issue #1747
Given high fee environment many users run into issue with default fee rate of 1.

Review checklist:

@RarePepeMaximalist
Copy link

This looks good to me -- I think we should also make --fee-rate a required parameter on ord inscribe ?

Also we should update the guide to use --fee-rate in the example
https://github.com/casey/ord/blob/master/docs/src/guides/inscriptions.md

@veryordinally
Copy link
Collaborator Author

Updated the guide, thanks @RarePepeMaximalist for the pointer.

Update for ord inscribe makes sense to me for consistency.

@droplister
Copy link

droplister commented Feb 15, 2023

Please include in your review this piece of code. If you are sending an amount of sats rather than an inscription, it will use an RPC command that does not take a fee_rate at all, but relies on the default behavior of send_to_address.

I think this means that even if you require a --fee-rate parameter, it will not be utilized in this instance. So, the program will complain about a fee rate not being set and when you set one it won't use that fee rate provided which is worth fixing later.

https://github.com/casey/ord/blob/b0ca48658d1b5eb911e1a2e3afea675134f9ba4d/src/subcommand/wallet/send.rs#L51-L74

@casey casey merged commit 3b49ab2 into ordinals:master Feb 15, 2023
@casey
Copy link
Collaborator

casey commented Feb 15, 2023

Great point @droplister! This is actually the root problem behind #1432. So extremely nice find!

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.

5 participants