Skip to content

Monero fee improvement attempt - #1

Open
SamsungGalaxyPlayer wants to merge 37 commits into
btcpay-monero:masterfrom
SamsungGalaxyPlayer:monero-fee
Open

Monero fee improvement attempt#1
SamsungGalaxyPlayer wants to merge 37 commits into
btcpay-monero:masterfrom
SamsungGalaxyPlayer:monero-fee

Conversation

@SamsungGalaxyPlayer

Copy link
Copy Markdown

The existing code creates a Monero fee in the UI of 1.9E-09 XMR. This is substantially less than a normal Monero network fee.

The fee response from get_fee_estimate is the "Amount of fees estimated per byte in atomic-units".

https://docs.getmonero.org/rpc-library/monerod-rpc/?h=get_fee_estimate#get_fee_estimate

I thus change feeRatePerKb to feeAtomicRatePerByte, and I use feeRatePerByte to convert the fee in atomic units to non-atomic. Then I multiply this value by 1500 bytes, which is currently a reasonable approximate weight of a 1-in/2-out Monero transaction.

Apologies if I missed anything in this PR!

The existing code creates a Monero fee in the UI of 1.9E-09 XMR. This is substantially less than a normal Monero network fee.

The `fee` response from `get_fee_estimate` is the "Amount of fees estimated per byte in atomic-units".

https://docs.getmonero.org/rpc-library/monerod-rpc/?h=get_fee_estimate#get_fee_estimate

I thus change `feeRatePerKb` to `feeAtomicRatePerByte`, and I use `feeRatePerByte` to convert the fee in atomic units to non-atomic. Then I multiply this value by 1500 bytes, which is currently a reasonable approximate weight of a 1-in/2-out Monero transaction.
@NicolasDorier

NicolasDorier commented Apr 7, 2025

Copy link
Copy Markdown
Contributor

hey @SamsungGalaxyPlayer thank you for this PR. We are releasing the 2.1 very soon, I want then to hand off this repo to the monero community which can merge that.

Just to make sure I don't introduce a bug as we are going to release, I don't merge it myself.

Excited to see you guys going freely on the monero integration! :)

napoly and others added 9 commits April 11, 2025 13:31
Co-authored-by: Chukwuleta Tobechi <47084273+tchukwuleta@users.noreply.github.com>
* Add unit tests for MoneroLikeConfiguration and MoneroLikePaymentData

* Move unit tests before playwright install

* Add unit tests for MoneroMoney and MoneroPaymentViewModel

* Refactor GitHub Action workflow to initialize submodules and clean project before build
…l testing (btcpay-monero#12)

* Add unit tests for services and update project references for internal testing

* Remove  file path comment from ParseStringConverterTest.cs
deverickapollo and others added 10 commits May 25, 2025 06:00
* Add run-id parameter to coverage

* Add github token to coverage workflow

* Add run-id to coverage

* Update download-artifact action to version 4.3.0

* bump upload-artifact action to v4.6.2
* Updating ReadMe because it's sloppy

* Updated ReadMe with detailed instructions

* Title and Updates

* Continued changes

* Removed section

* Uodated Readme

* added code line

---------

Co-authored-by: XMRpriest <83054266+FreedomCrypto00@users.noreply.github.com>
@deverickapollo

Copy link
Copy Markdown
Member

@SamsungGalaxyPlayer - Can you resolve the merge conflict here? I'll pull this in with our next release.

@napoly

napoly commented Jun 28, 2026

Copy link
Copy Markdown
Member

Hey @SamsungGalaxyPlayer .. thank you for this PR! We finally have all the infrastructure up and ready to bring this in safely.. can you pls rebase ?

git fetch origin
git rebase -i origin/master
# fix any conflicts, squash.. then:
git push -f

As for the code you are casting long/double.. losing precision but I believe this will be pointed out for you by CodeRabbit once rebased. I also don't like hard-coded values --> 1500.. but this is an estimation.. could be calculated to some degree but doesn't seem to be worth it..

We do have an integration tests that hit this part of code.. fyi.. but as this is a calculation it should be covered by unit test/s i think (again AI can help a lot.. one should mock away the rpc response..)

Pls let us know if you get stuck!

@nahuhh

nahuhh commented Jun 28, 2026

Copy link
Copy Markdown

I also don't like hard-coded values --> 1500.. but this is an estimation.. could be calculated to some degree but doesn't seem to be worth it..

you cant know the input select before generating the tx.

afaik, the only way to know an accurate fee is to submit the proposed tx using do_not_relay, which will return the correct/exact fee. (it will also submit the tx to the node w/ a do_not_relay flag attached to it. I believe this tx can still be mined, but wont be relayed)

there is also a get_fee_priority method that will tell you which fee lvl / multiplier that "automatic" tier is using.

@nahuhh

nahuhh commented Jun 28, 2026

Copy link
Copy Markdown

Also, i might not suggest a rebase. This has been fudged up with multiple merge commits.

probably easier to just git reset --hard HEAD~2 to drop the merge commits, then rebase.

or to cherry-pick the first 2 commits onto a clean branch off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

7 participants