Skip to content

Replacing fee_rate with sat_per_vbyte#2054

Open
sergey3bv wants to merge 2 commits into
lightninglabs:mainfrom
sergey3bv:fix/fee-rate-name
Open

Replacing fee_rate with sat_per_vbyte#2054
sergey3bv wants to merge 2 commits into
lightninglabs:mainfrom
sergey3bv:fix/fee-rate-name

Conversation

@sergey3bv
Copy link
Copy Markdown
Contributor

Should close #1661

@gemini-code-assist
Copy link
Copy Markdown

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request improves the clarity of the API by renaming a fee-related field to explicitly include its unit of measurement. It also corrects the documentation to accurately describe the transaction type associated with this fee rate.

Highlights

  • API Renaming: Renamed the fee_rate field to fee_rate_sat_kw in the SendAssetRequest API to better reflect the unit of measurement.
  • Documentation Update: Updated the field description to clarify that the fee rate applies to the anchor transaction rather than the minting transaction.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request renames the 'FeeRate' field to 'FeeRateSatKw' in the 'SendAssetRequest' struct and its associated methods to improve clarity regarding the unit of measurement. The changes are applied across the codebase, including command-line assets, integration tests, and the RPC server. I have no further feedback to provide.

@darioAnongba darioAnongba self-assigned this Apr 7, 2026
Copy link
Copy Markdown
Contributor

@darioAnongba darioAnongba left a comment

Choose a reason for hiding this comment

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

Hi! Sadly this PR is not doing what is expected:
Issue #1661 explicitly requests renaming fee_rate to sat_per_vbyte to match lnd's SendCoinsRequest convention. The PR title says the same. But the actual implementation renames to fee_rate_sat_kw (sat per kiloweight)

The behaviour should be the same as FundChannelRequest (on the CLI as well).

@github-project-automation github-project-automation Bot moved this from 🆕 New to 👀 In review in Taproot-Assets Project Board Apr 8, 2026
@sergey3bv
Copy link
Copy Markdown
Contributor Author

Hi! Sadly this PR is not doing what is expected: Issue #1661 explicitly requests renaming fee_rate to sat_per_vbyte to match lnd's SendCoinsRequest convention. The PR title says the same. But the actual implementation renames to fee_rate_sat_kw (sat per kiloweight)

The behaviour should be the same as FundChannelRequest (on the CLI as well).

My bad. I will fix it as soon as possible.

@sergey3bv sergey3bv force-pushed the fix/fee-rate-name branch from ecca3fb to e190657 Compare April 9, 2026 07:45
@sergey3bv sergey3bv requested a review from darioAnongba April 9, 2026 07:45
@sergey3bv sergey3bv force-pushed the fix/fee-rate-name branch 2 times, most recently from 6f4eaa8 to 25067b4 Compare April 9, 2026 11:22
Copy link
Copy Markdown
Contributor

@darioAnongba darioAnongba left a comment

Choose a reason for hiding this comment

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

We can either also do the migration for FundBatch and FinalizeBatch to use sat/vB with the same pattern or implement it in follow-up PRs.

Comment thread taprpc/taprootassets.proto
Comment thread taprpc/taprootassets.proto Outdated
Comment thread docs/release-notes/release-notes-0.8.0.md
Comment thread itest/addrs_test.go Outdated
Comment thread rpcserver/rpcserver.go Outdated
@sergey3bv
Copy link
Copy Markdown
Contributor Author

We can either also do the migration for FundBatch and FinalizeBatch to use sat/vB with the same pattern or implement it in follow-up PRs.

Well, for now I will focus on your comments and later do the migration.

@darioAnongba
Copy link
Copy Markdown
Contributor

Hi! Just mentioning that CI is red and branch could be rebased.

@lightninglabs-deploy
Copy link
Copy Markdown

@darioAnongba: review reminder
@sergey3bv, remember to re-request review from reviewers when ready

@sergey3bv sergey3bv force-pushed the fix/fee-rate-name branch from e824b31 to 180b2df Compare May 8, 2026 12:36
@sergey3bv
Copy link
Copy Markdown
Contributor Author

Hey, @darioAnongba, I rebased the PR to the latest main, could you please take a look

@sergey3bv sergey3bv force-pushed the fix/fee-rate-name branch 2 times, most recently from 6bd541a to 76a5042 Compare May 12, 2026 12:22
Copy link
Copy Markdown
Contributor

@darioAnongba darioAnongba left a comment

Choose a reason for hiding this comment

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

thanks! Very close now. Only small details.

Comment thread itest/custom_channels/liquidity_test.go Outdated
Comment thread itest/custom_channels/liquidity_test.go Outdated
Comment thread itest/custom_channels/helpers.go Outdated
Comment thread itest/custom_channels/helpers.go Outdated
Comment thread itest/custom_channels/helpers.go Outdated
Comment thread taprpc/taprootassets.proto Outdated
repeated AddressWithAmount addresses_with_amounts = 5;

// The optional fee rate to use for the anchor transaction, in sat/vB.
uint64 sat_per_vbyte = 8;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint64 sat_per_vbyte = 8;
uint64 sat_per_vbyte = 6;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like this specific change breaks CI tests

@sergey3bv sergey3bv force-pushed the fix/fee-rate-name branch from 76a5042 to 0a2ee23 Compare May 12, 2026 14:04
@sergey3bv sergey3bv requested a review from darioAnongba May 13, 2026 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

[bug]: SendAssetRequest: change fee_rate to sat_per_vbyte

3 participants