-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Example orderbook subscribe sol. #19
base: main
Are you sure you want to change the base?
Example orderbook subscribe sol. #19
Conversation
initial example adapted from max getting lowest ask using wss to subscribe to bids and asks moved the subscription code to own class for reuse added orderbook class which takes book sides and logs the info on updates from either of the book side added a reusable class for wss boilerplate and a subscription class for trade event queue refactored the example so the subscription classes can be reused by other clients atomic snapshot for orderbook struct to avoid potential data race using bsp instead of bps. atomic variables for bookside and added empty calls for book depth getting all leaf nodes and sorting them as per prices const correctness at solana api added market depth logic further refactoring midpoint double replaced atomic with mtx due to issues at macos, can be investigated later simplified the fill reception callback
|
||
websocketpp::lib::error_code ec; | ||
ws_client::connection_ptr con = c.get_connection( | ||
"wss://mango.rpcpool.com/946ef7337da3f5b8d3e4a34e7f88", ec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a new constant to mango.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, MAINNET.endpoint should work.
output should probably display ui units instead of native units, i'll open a separate ticket for this |
include/subscriptions/bookSide.hpp
Outdated
if (!newOrders.empty()) { | ||
{ | ||
std::scoped_lock lock(ordersMtx); | ||
orders = std::move(newOrders); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newOrders should already be sorted by price as the order book tree data structure is sorted by price
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. will have to make sure though that the bidside and askside sorting is from midpoint to either ends.
include/subscriptions/bookSide.hpp
Outdated
leafNode->timestamp + leafNode->timeInForce < nowUnix; | ||
if (isValid) { | ||
newOrders.emplace_back((uint64_t)(leafNode->key >> 64), | ||
leafNode->quantity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have a few issues with the way the code is organized:
- parsing the order tree should be independent from how the data is transported (rpc / ws), the code should hence not live under subscriptions
- same is true for getVolume
- each LeafNode actually contains a lot of valuable information that is discarded here in favor of the very incomplete order struct, this makes this code very hard to re-use (we have an order type in most sdks, it usually contains more data then the LeafNode, ui values for instance)
- i think there's value in a generic
accountSubscriber<typename T>
that stores anstd::shared_ptr<T>
so data can be accessed concurrently without the use of critical sections. just let a public method return new shared_ptr instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will ping you on discord to catch up here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::shared_ptr so data can be accessed concurrently
where only headers of shared_ptr
are atomic. stuff it holds isn't.
tried it locally and the code works well for a few minutes but then one by one the different sockets disconnect. once a single socket disconnects, aggregated data can be invalid (e.g. mid price, after one side of the book stopped updating) |
@@ -135,8 +138,10 @@ struct EventQueueHeader { | |||
uint64_t seqNum; | |||
}; | |||
|
|||
// todo: change to scoped enum class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, want to open a new issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. #22
* moving book parsing code out of subscriber class
…p logging * some refactoring
* removed unnecessary structs
c4bca17
to
06c7e5d
Compare
re your comment #19 (comment):
I have one point about the structure of the code overall: |
include/subscriptions/orderbook.hpp
Outdated
} | ||
|
||
private: | ||
std::shared_ptr<L1Orderbook> level1 = std::make_shared<L1Orderbook>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullptr makes sense here to differentiate between waiting for first data update vs empty book
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. will change it.
include/mango_v3.hpp
Outdated
if ((*iter).tag == NodeType::LeafNode) { | ||
const auto leafNode = | ||
reinterpret_cast<const struct LeafNode *>(&(*iter)); | ||
const auto now = std::chrono::system_clock::now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you dont want to recalculate the current time inside the loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 36 in 9deb4c9
const auto now = std::chrono::system_clock::now(); |
mango gravy from the initial example lol.
pls create a seperate bug/issue for this.
include/mango_v3.hpp
Outdated
|
||
private: | ||
uint64_t lastSeqNum = INT_MAX; | ||
std::shared_ptr<uint64_t> latestTrade = std::make_shared<uint64_t>(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just an uint64_t? trade at price 0 is impossible after all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need a mtx/atomic otherwise. same reason why we replaced all mtx'es with shared ptrs.
event based synchronization between getLastTrade and handleMsg.
can change to nullptr though so client of getLastTrade could parse the validity of returned value.
include/mango_v3.hpp
Outdated
// all other messages are event queue updates | ||
const std::string method = msg["method"]; | ||
const int subscription = msg["params"]["subscription"]; | ||
const int slot = msg["params"]["result"]["context"]["slot"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not correct, slots here are related to the chain consensus not the ringbuffer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well the gravy for mango related code was taken from the original examples from the first commit.
solcpp/examples/accountSubscribe.cpp
Line 69 in dd58f26
const int slot = parsedMsg["params"]["result"]["context"]["slot"]; |
you can create a bug/issue for this. out of scope for this imo.
include/mango_v3.hpp
Outdated
public: | ||
BookSide(Side side) : side(side) {} | ||
|
||
bool handleMsg(const nlohmann::json &msg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should really live in the websocket subscription code, rather than the account type. in fact the code should work the same for all types of account subscriptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first four lines to check json "result" or the whole of msg handling?
first four lines could be moved one layer below to wss subscription code in case we know that each msg has the "result" object.
the other parsing code must be handled by the account itself.
# Conflicts: # examples/CMakeLists.txt # include/mango_v3.hpp # include/solana.hpp
… type itself to interpret
added bookdelay mechanism
…use the struct whichever way it wants
updated the PR with following changes:
|
std::to_string(decoded.size()) + " expected " + | ||
std::to_string(sizeof(BookSideRaw))); | ||
} | ||
memcpy(&(*raw), decoded.data(), sizeof(BookSideRaw)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs critical section from before memcpy to end of iterator lifetime.
concurrent update calls else could cause memory corruption.
another option would be to not update the same buffer and to swap references to a new buffer, so that iterator can still iterate over the "old buffer" and de-allocate it on iterator destruction.
include/mango_v3.hpp
Outdated
uint64_t getVolume(uint64_t price) const { | ||
Op operation; | ||
uint64_t volume = 0; | ||
for (auto&& order : *orders) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
orders needs to be dereferenced once before entering loop
|
||
try { | ||
websocketpp::lib::error_code ec; | ||
ws_client::connection_ptr con = c.get_connection(MAINNET.endpoint, ec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAINNET.endpoint
should not be statically passed but we should create an endpoint argument in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again:
std::shared_ptr so data can be accessed concurrently
if it's so it's a bad thing. Is it?
this example subscribes to SOL perp market on-chain events for bids, asks and fill.
following info is logged at each update of orderbook as well as fill event:
note: in case no fill event received before an orderbook update, the latest trade will be empty. maybe there is a way to get the last traded price(mango-bowl recent trades maybe?), in that case we can add that in future.
snippet:
additions:
minor improvements: