XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.54k stars 1.47k forks source link

Remove "fee units" from API #3164

Open mDuo13 opened 5 years ago

mDuo13 commented 5 years ago

Fee units are used internally to calculate fees with higher precision than integer drops, but are exposed directly in the ledger subscription stream (possibly other places, though I don't see any immediately). These fields are more confusing than helpful; and in the ledger stream at least the exact XRP values (which are actually useful) are reported anyway.

Fee units should be an "internal only" property that is not exposed in the API.

Note, "fee units" are different from "fee levels". I'm willing to entertain the argument that "fee levels" should also be removed because they are confusing, but "fee levels" do have some use to API users, whereas "fee units" don't. Fee levels show up in the queuing endpoints, and compare the relative costs of transactions with different minimum fees. That's important for knowing which transactions will be queued or evicted from the queue first. For example, an EscrowFinish transaction containing a 32-byte crypto-condition preimage paying the minimum of 350 drops is ranked lower than a basic AccountSet transaction paying a "double" cost of 20 drops.

mDuo13 commented 4 years ago

Using this commit as a template, I think the fix for this is to wrap both of the following fee_ref lines from NetworkOPs in a condition kind of like this:

if (context.apiVersion == 1)

In subLedger(): https://github.com/ripple/rippled/blob/0b4e34b03b72196510998375fd28622c8b63aaa7/src/ripple/app/misc/NetworkOPs.cpp#L2887

In pubLedger(): https://github.com/ripple/rippled/blob/0b4e34b03b72196510998375fd28622c8b63aaa7/src/ripple/app/misc/NetworkOPs.cpp#L3287

I started to make the commit myself but realized I don't know the appropriate way to get the right context into the subscription info, especially since multiple clients could be subscribed using different API versions.

mvadari commented 1 year ago

Is this resolved by XRPFees?

ximinez commented 1 year ago

Is this resolved by XRPFees?

I think so.

intelliot commented 1 year ago

We can keep this issue open until XRPFees successfully activates (and @mDuo13 confirms that it fully resolves this issue)