Skip to content
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

transactions: add big comment about offers and the order book. #2027

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Mar 27, 2019

This adds a big explanatory comment about how the order book works and what the general ideas behind the limit and exact offers in the system are all about. I wrote it because the order book code is pretty opaque to me still, and I really couldn't make heads or tail of CAP-0006 without this level of pedantic self-explanation. Figured it might be useful to others.

I would love a thorough read-through and correction of it if possible from @jonjove and/or @MonsieurNicolas because I'm only very very moderately convinced I got it right. I think the commentary hangs together in the sense that I think it's painting a logically-consistent explanation for things, but I am much less sure that it's correct with respect to the code.

Apologies for the level of repetitiveness / redundancy of phrasing / pictures / etc. I know it's probably overkill for people familiar with the subsystem and ideas, but for me I really get completely lost every time I dig into it. Wanted a very explicit guide.

@MonsieurNicolas
Copy link
Contributor

I'll take a look at this a bit later. Glancing at it, most should probably go in the "distributed exchange" docs https://github.com/stellar/docs/blob/master/guides/concepts/exchange.md as any user (not just stellar-core devs) should understand it.

You can leave the technical bits about where in the code things live in comments.

Copy link
Contributor

@vogel vogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, one typo to fix

Copy link
Contributor

@MonsieurNicolas MonsieurNicolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added an extra comment after reviewing, you may also be interested in the comment I just made in the cap006 PR #2016 (comment)

Repeating what I said earlier, I think this should be moved to the docs repo: it's a good intro for the order book that should be pushed to all devs (not just core devs).

src/transactions/OfferExchange.h Outdated Show resolved Hide resolved
@oleganza
Copy link

oleganza commented May 2, 2019

this patch is very illuminating, thanks.

@graydon
Copy link
Contributor Author

graydon commented May 4, 2019

I split the first half (orientation) off to stellar-deprecated/docs#453 against the docs repo, and fixed the associated suggestions here. Anything else?

@MonsieurNicolas
Copy link
Contributor

just noticed that you have a libmedida change in there, probably not what you wanted to do

@graydon
Copy link
Contributor Author

graydon commented May 7, 2019

@MonsieurNicolas whoops! nice catch thanks, reverted the medida change.

@MonsieurNicolas MonsieurNicolas added this to In progress in v12.0.0 via automation Aug 9, 2019
@MonsieurNicolas MonsieurNicolas removed this from In progress in v12.0.0 Sep 18, 2019
@MonsieurNicolas MonsieurNicolas added this to In progress in v12.1.0 via automation Sep 18, 2019
@MonsieurNicolas MonsieurNicolas removed this from In progress in v12.1.0 Sep 24, 2019
@MonsieurNicolas MonsieurNicolas added this to In progress in v12.0.0 via automation Sep 24, 2019
@MonsieurNicolas MonsieurNicolas merged commit 019e932 into stellar:master Sep 24, 2019
v12.0.0 automation moved this from In progress to Done Sep 24, 2019
@graydon graydon deleted the orderbook-comment branch January 3, 2020 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v12.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants