Finalize autobridging implementation (RIPD-179): #689
Conversation
|
You're gonna need a much bigger commit message than that |
| class Rate | ||
| { | ||
| private: | ||
| std::uint32_t quality_; |
There was a problem hiding this comment.
Sorry for always starting with quibbles while I read through the whole thing.
I think you could make both of these members const...
There was a problem hiding this comment.
I'm not really a fan of const data members. The class becomes unassignable where previously it could have been assigned.
|
I have rebased the code against the latest develop (sorry, I had to - I didn't want to be too far out of date) and have addressed some of the concerns from the preliminary review from you guys and in-person reviews at the office. |
| return original_; | ||
| } | ||
|
|
||
| // TODO: the presence of 'output' is an artifact caused by the fact that |
There was a problem hiding this comment.
This comment adds mystery rather than removes it. You seem to use all of output, yes?
There was a problem hiding this comment.
I don't really understand why the issue is at, cough, issue at all in this code. You're just performing a division. Is that what you're referring to? If so, get rid of the asserts - if you don't believe that issue should be there, you shouldn't complain if it's wrong! :-)
There was a problem hiding this comment.
Why not call it multiplyAndClamp or multiplyAndLimit - you're doing two things (multiplying and taking an std::min) not just multiplying (or dividing).
There was a problem hiding this comment.
Explicitly documenting the clamp would be a good idea, thanks.
|
Ping!! We have 19 pull requests on the queue... |
* RIPD-486: Refactoring the taker into a unit-testable architecture * RIPD-659: Asset-aware offer crossing * RIPD-491: Unit tests for IOU to XRP, XRP to IOU and IOU to IOU * RIPD-441: Handle case when autobridging over same owner offers * RIPD-665: Handle case when autobridging over own offers * RIPD-273: Groom order books while crossing Additionally, add support in LedgerEntrySet to allow transfer fees to be specified by calling code instead of being calculated by the transfer functions: * Implement new "issue/redeem" model for IOU movements * Implement new "transfer" model for IOU movements Clean up signature verification logic in the main Transactor.
|
So you seem to have rebased and squashed into one commit. It would have been helpful to have changes be a separate commit, 'cause now we have to review the whole thing again... :-( |
|
It's just rebased - there are no functional changes; only comments and a gratuitous rename. |
|
Need a much bigger commit message. Also can the LedgerEntrySet change be pulled out into its own earlier commit with a descriptive message? |
|
Vinnie, I'll see about pulling the issue/redeem stuff out. Actually, I think that most of that might have already been pulled into develop and this just begins using the API. I'll update accordingly. |
|
So I was pretty good with what's there, so I'm really interested in either seeing the issue/redeem stuff pulled out, or seeing this checked in. |
|
@rec This changes transaction processing so its not safe to merge in active form. Although, I'm strongly in favor of merging it but with a preprocessor setting to have it turned off. |
|
This changes transaction processing whether autobridging is enabled or not. I will pull the |
|
Leaving this here as a reminder: https://github.com/sublimator/rippled/commits/autobridge-tests |
|
Seems like |
|
This branch is out of date - I don't want to rebase it and spoil the review. For a rebased version, check out https://github.com/nbougalis/rippled/tree/txchanges |
|
Thank you :) |
|
Where are we with this? Rebasing works much better these days!! |
|
I already have this and all other TxChanges rebased and ready to go. |
|
Congrats |
Additionally, add support in LedgerEntrySet to allow transfer fees
to be specified by calling code instead of being calculated by the
transfer functions:
Clean up signature verification logic in the main Transactor.
Reviews: @JoelKatz, @rec, @vinniefalco but everyone is welcome to get in on the game.