rippled
rippled copied to clipboard
Introduce `fixNegativeSpreadsV1` amendment
High Level Overview of Change
This amendment builds on the fixRmSmallIncreasedQOffers amendment. It introduces the method minPrecisionSafeAmountIn on the TOffer class. The minimum safe amounts currently are defined as:
IOU -> XRP = 1000000000000000e-96
XRP -> IOU = 1000 (drops)
IOU -> IOU = 1000000000000000e-96
These values are being used as new thresholds for the existing fixRmSmallIncreasedQOffers logic. How exactly this solves the negative spreads issue is explained here: https://github.com/XRPLF/XRPL-Standards/discussions/74
Type of Change
- [x] Bug fix
- [x] Breaking change
Test Plan
An automated test script can be found at https://github.com/Mwni/xrpl-tests/tree/master/negative-spread. Ideally, this should be ran against a validator with, and without this amendment.
I force pushed a signature. The first push was by mistake, it missed the proper GPG key. The second has it.
@Mwni, thanks for the effort you put into this pull request. Also sorry that this amendment has been sitting so long.
Your pull request prompted me to do some research into the computational source of these offers that block books. I have some results that I think are interesting, and they also suggest a different approach to solving the problem. But I don't have code done yet.
I'd like to ask for an additional three weeks of patience (I'll be on vacation next week) while I attempt to code up an alternative approach. After I've coded that up we can look at both approaches and decide which is better. If I can't make that three week deadline (August 3, 2022) then I'll stop blocking you.
That said, in my opinion it would be imprudent to put an amendment in the code base without some kind of unit test. So this pull request needs unit tests. Don't code up that test yet. I'll be producing unit tests as I work on the other approach. If we end up not using the approach I'm working on then at least you can leverage the unit tests that I produce.
Thanks again for your patience.
@scottschurr alright. It will be interesting to see a different approach. Great Holiday, and talk to you in August!
Hi @Mwni, thanks for your patience. I have an alternative proposal to this pull request here: https://github.com/XRPLF/rippled/pull/4264
Personally, I'm more comfortable with the approach taken by https://github.com/XRPLF/rippled/pull/4264 because the ledger simply stops producing offers that block order books. So, assuming I've found all of the situations where order book blocking offers are produced, there is no need to increase the threshold for shouldRmSmallIncreasedQOffer(); there should no longer be any increased quality offers.
There is, of course, no guarantee that the alternative approach will pass code review. Or it's possible other reviewers will prefer the approach you've taken. So it makes sense to leave this pull request open until that decision is resolved.
@scottschurr Yes, I agree. #4264 is a far better solution. With my little experience, I wouldn't have dared to touch the ledger's internal arithmetics. But you definitely can.
Closing because this PR is superseded by https://github.com/XRPLF/rippled/pull/4264.