Separate fuzzer setup parameters from operations #2897
Conversation
src/test/FuzzerImpl.cpp
Outdated
| // and an account with a bid and a sell. It also gives us an initial | ||
| // order book that is in a legal state. | ||
| std::array<OfferParameters, 24> constexpr orderBookParameters{ | ||
| {OfferParameters{AssetID(), AssetID(1), 13, 3, 2, 10}, // asset pair 0 |
There was a problem hiding this comment.
I think we should try to make this section look as much as possible like the comment above (and that comment should be removed as it's wrong anyways):
- use an empty line between order book pairs
- price should be the last argument, and price should be sorted so that we can clearly see how the order book is structured (note that because price is in terms of sell/bid, the sort order flips when switching buy and sell)
- I think you can get rid of the
OfferParameters? (or maybe this is C++ 17)
There was a problem hiding this comment.
Each section we should have some comment that explains which property we're trying to achieve.
To do this, we may have to do the same for accounts, that right now are setup pretty much all the same (instead, we probably want to mix things up a bit in terms of limits on balance entries and assets trusted).
Note: as we're trying to match the coverage from master, we may need less accounts and offers variations than what we're doing now.
There was a problem hiding this comment.
and that comment should be removed as it's wrong anyways
I left the part that documents the initial state of the order book, but removed the stuff about it being a "good mixture".
There was a problem hiding this comment.
use an empty line between order book pairs
Done.
There was a problem hiding this comment.
price should be the last argument, and price should be sorted so that we can clearly see how the order book is structured (note that because price is in terms of sell/bid, the sort order flips when switching buy and sell)
I've made Price into the last argument (and otherwise brought the argument order into line with the comment). I think it was already in best-offer order? I might be misunderstanding that.
There was a problem hiding this comment.
I think you can get rid of the OfferParameters? (or maybe this is C++ 17)
Oh yes, you're right. I thought it was C++17, but clang at least is allowing me to do it with std=c++14.
There was a problem hiding this comment.
Each section we should have some comment that explains which property we're trying to achieve.
To do this, we may have to do the same for accounts, that right now are setup pretty much all the same (instead, we probably want to mix things up a bit in terms of limits on balance entries and assets trusted).
Note: as we're trying to match the coverage from master, we may need less accounts and offers variations than what we're doing now.
Agreed on all points -- I wanted to start with just the order book so that we could get consensus on how the parameter-refactoring should look, and then once that was merged, I'd do the same for accounts, and for the other structures that I'm adding to setup. I also definitely plan to change the setup parameters themselves, but I wanted to separate the mechanism changes from the policy changes.
There was a problem hiding this comment.
I think this is good now. I'm going to merge it tomorrow
There was a problem hiding this comment.
I think this is good now. I'm going to merge it tomorrow
Thanks. I've rebased onto master.
rokopt
force-pushed
the
fuzzer-setup-refactor-parameters
branch
2 times, most recently
from
df9cd7c
to
d56c5f9
Compare
|
I think I've handled all code-review comments; how does the change look now? |
Factor out the specifics of which offers to create into a single structure, which can be examined at a glance and changed in isolation, from the operations which establish that order book structure during fuzzer setup.
rokopt
force-pushed
the
fuzzer-setup-refactor-parameters
branch
from
d56c5f9
to
9ef9dbc
Compare
|
r+ 9ef9dbc |
Separate fuzzer setup parameters from operations
Part of #1376
Could be the beginning of a foundation for #2891 , #2892 , #2894
Factor out the specifics of which offers to create into a
single structure, which can be examined at a glance and
changed in isolation, from the operations which establish
that order book structure during fuzzer setup.
We plan to set up the order book to be more varied and less symmetrical than it currently is, but I think that we should separate working out how to factor out the parameters to make that kind of change simpler to perform and understand from actually making those changes. So in this commit, I'm not changing the initial order book, only how the setup is specified (this commit makes it declarative, and
constexpr).Checklist
clang-formatv5.0.0 (viamake formator the Visual Studio extension)