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

wallet: add fee rate estimation to FundRawTx #302

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

YusukeShimizu
Copy link
Contributor

@YusukeShimizu YusukeShimizu commented Jul 18, 2024

This is a fix for the inconvenience caused by the FundRawTx , which sometimes set relatively high fees.

Add getFeeRate function to estimate optimal fee rate based on network conditions.
Replace FundRawTx with FundRawWithOptions to support fee rate options.

@grubles
Copy link
Collaborator

grubles commented Jul 18, 2024

@nepet Can you take a look?

@YusukeShimizu YusukeShimizu force-pushed the wallet/fee-rate-estimatio branch from c949103 to 6904f99 Compare July 18, 2024 03:00
@YusukeShimizu
Copy link
Contributor Author

In order to address this issue, I reverted to draft status

https://discord.com/channels/905126649224388629/905126649698320406/1265218998241591412

@YusukeShimizu YusukeShimizu marked this pull request as draft July 25, 2024 09:13
Copy link
Contributor

@nepet nepet left a comment

Choose a reason for hiding this comment

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

I think this PR is fine and I would merge it as is and start a new one to figure out the 1000 sat problem discussed in discord https://discord.com/channels/905126649224388629/905126649698320406/1265218998241591412.
*whatever you prefere is fine tho ^^ !

ack 6904f99

wallet/elementsrpcwallet.go Show resolved Hide resolved
@YusukeShimizu
Copy link
Contributor Author

YusukeShimizu commented Jul 28, 2024

Thank you for your review!
For ease of test, I addressed this in this PR.

The pre-conditions in swap in and out have been updated to ⁠wallet_balance_sat > swap_amount_sat + estimated_fee.
Additionally, I have integrated this check into the swap package layer as part of a refactor.
a973fab

I have tested locally.

./out/pscli lbtc-getbalance
{
  "sat_amount":  "1000000"
}
./out/pscli swapin --channel_id 120946279120897 --sat_amt 1000000 --asset lbtc
2024/07/28 13:51:50 rpc error: code = Unknown desc = exceeding maximum swap amount: 999700
./out/pscli swapin --channel_id 120946279120897 --sat_amt 999700 --asset lbtc
{
  "swap":  {
    "id":  "dd926863b50200f0e5ccf4311ebbb5b0ea20e96c9a709ea2e45c72919b891ed8",
    "created_at":  "1722142344",
    "asset":  "lbtc",
    "type":  "swap-in",
    "role":  "sender",
    "state":  "State_SwapInSender_SendTxBroadcastedMessage",
    "initiator_node_id":  "0328bf2621a6ed6b4c6039c6ffe421bf8b1700b5e8136b717f9fa7953bc06eaafb",
    "peer_node_id":  "02103606f048e9680b03e70e3e7bea6ca3e4619dcfd9104112e95835845fdf556d",
    "amount":  "999700",
    "channel_id":  "110:1:1",
    "opening_tx_id":  " ",
    "claim_tx_id":  "",
    "cancel_message":  "",
    "lnd_chan_id":  "120946279120897"
  }
}
./bin/elements-cli -rpcwallet=peerswap1 getrawtransaction b6d03a002e2907efc5df7dc6f5e1c61ffac29ef6a03ea4d49c9ddd32011a6c09 true | jq -r '.vsize, .fee["5ac9f65c0efcc4775e0baec4ec03abdde22473cd3cf33c0419ca290e0751b225"]' | {
  read -r vsize;
  read -r fee;
  fee_satoshi=$(echo "$fee * 100000000" | bc);
  feerate=$(echo "scale=2; $fee_satoshi / $vsize" | bc);
  echo "Fee Rate: $feerate sat/vB";
  echo "Fee: $fee_satoshi sat";
}
Fee Rate: .22 sat/vB
Fee: 300.00000000 sat

@YusukeShimizu YusukeShimizu marked this pull request as ready for review July 28, 2024 07:54
@YusukeShimizu YusukeShimizu requested a review from nepet July 28, 2024 22:51
Copy link
Contributor

@nepet nepet left a comment

Choose a reason for hiding this comment

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

lgtm! I appreciate that you also clean-up and refactor old stuff on the way.
Please squash the fixup commit before before merging (Let me know if you want me to squash and merge).

This is a fix for the inconvenience caused by the default setting
 of fund raw tx, which sometimes set relatively high fees.

add getFeeRate function to estimate optimal
fee rate based on network conditions.
Replace FundRawTx with FundRawWithOptions to support fee rate options.
Fix pre-conditions in swap in and out,
to `wallet_balance_sat > swap_amount_sat + estimated_fee

This prevents change amounts being unexpectedly posted to dust.
I have also integrated the check into the swap
package layer as a refactor.
@YusukeShimizu YusukeShimizu force-pushed the wallet/fee-rate-estimatio branch from d1e54b5 to 9dc5c51 Compare August 2, 2024 01:49
@YusukeShimizu YusukeShimizu merged commit 5935fb4 into master Aug 2, 2024
8 checks passed
@YusukeShimizu YusukeShimizu deleted the wallet/fee-rate-estimatio branch August 2, 2024 02:02
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.

3 participants