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

API documentation #780 #1174

Merged
merged 15 commits into from Oct 6, 2018

Conversation

5 participants
@cogutvalera
Copy link
Member

commented Jul 25, 2018

PR for #780

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

nice job, ill take a look in more detail as soon as i can.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

Thank you very much ! Really appreciate all your efforts and help !

@abitmore
Copy link
Member

left a comment

Need to improve. Thanks anyway.

* @param a Base in pair
* @param b Quote in pair
* @param limit Maximum number of orders to retrieve
* @return A list of orders with b:a pair

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 25, 2018

Member

It returns history, not orders.

* @param bucket_seconds Represents the length in seconds of bucket
* @param start Time in seconds from where buckets begin
* @param end Time in seconds where buckets stop
* @return A list of markets with b:a pair (maximum markets to retrieve is less than 200)

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 25, 2018

Member

It returns history in one market, not markets. Descriptions of params seems off.

/**
* @brief Get holders for a specific asset
* @param asset_id The specific asset
* @param start The start index of first asset

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 25, 2018

Member

Seems wrong.

* @param asset_id The specific asset
* @param start The start index of first asset
* @param limit Maximum limit must not exceed 100
* @return A holders for the specified asset

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 25, 2018

Member

A holders sounds strange.


/**
* @brief Get tracked buckets
* @return A list of tracked buckets

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 25, 2018

Member

Perhaps need to explain what's a bucket.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

Thank you very much ! Will improve soon ! Sorry for mistakes !

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

improved, but there still maybe some wrong explanations, I will check myself again and will improve/fix what will find wrong.Thank you very much !

@abitmore abitmore added this to In progress in Feature release (201810) via automation Jul 29, 2018

@cogutvalera cogutvalera force-pushed the cogutvalera:valera_issue_780 branch from 5fef044 to e11e75a Jul 31, 2018

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

improved and rebased already

* @brief Get range proof info
* @param proof List of characters
* @return A range proof info structure with exponent, mantissa, min and max values
*/

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jul 31, 2018

Contributor

Do you understand how the crypto_api works?
Your documentation provides nothing useful that isn't obvious from the method/variable names.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 31, 2018

Author Member

not all details still researching this code in more details ... blind accounts and transactions are the main idea for crypto_api

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

I will improve this documentation more soon ...

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2018

Guys, thanks for your time and efforts ! Please check my new changes when you will have enough time for it. Should I make any other changes in api documentation or it is ok for now ?

Thanks !

@ryanRfox ryanRfox added this to In Testing in Community Claims Sep 4, 2018

@@ -343,8 +426,26 @@ namespace graphene { namespace app {
asset_api(graphene::chain::database& db);
~asset_api();

/**
* @brief Get account holders for a specific asset

This comment has been minimized.

Copy link
@pmconrad

pmconrad Sep 5, 2018

Contributor

"account holders" doesn't make sense. Either "asset holders" or "accounts holding asset".

vector<account_asset_balance> get_asset_holders( asset_id_type asset_id, uint32_t start, uint32_t limit )const;

/**
* @brief Get account holders count for a specific asset

This comment has been minimized.

Copy link
@pmconrad

pmconrad Sep 5, 2018

Contributor

Same.

vector<order_history_object> get_fill_order_history( asset_id_type a, asset_id_type b, uint32_t limit )const;

/**
* @brief Get markets with b:a pair

This comment has been minimized.

Copy link
@pmconrad

pmconrad Sep 5, 2018

Contributor

Gets accumulated market history, not current markets

* @param a Base in pair
* @param b Quote in pair
* @param limit Maximum number of orders to retrieve
* @return A history of orders with b:a pair, where a <= b, otherwise a and b will be swaped

This comment has been minimized.

Copy link
@pmconrad

pmconrad Sep 5, 2018

Contributor

If always a <= b then it doesn't make sense to call a "Base" and b "Quote". Backend is ignorant of UI market order.

* @param bucket_seconds Represents the length in seconds of bucket
* @param start Time in seconds from where buckets begin
* @param end Time in seconds where buckets stop
* @return A history in market with b:a pair (maximum markets to retrieve is less than 200), where a <= b, otherwise a and b will be swaped

This comment has been minimized.

Copy link
@pmconrad

pmconrad Sep 5, 2018

Contributor

Same.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2018

Your formatting changes seem to violate current conventions rather than improve things.

(Not that we're very consistent in that regard.)

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2018

I have made that code formatting only for those lines which exceed 130 characters

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Ok, keep your changes. Created #1318 for defining coding style.
@abitmore please re-review, I'm ok with the changes.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

Thanks !

@abitmore
Copy link
Member

left a comment

Note: I didn't check whether the docs about blinded transfers are correct because I know little about them.

@@ -176,10 +184,34 @@ namespace graphene { namespace app {
uint32_t stop = 0,
unsigned limit = 100,
uint32_t start = 0) const;

/**
* @brief Get history of orders with b:a pair

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

IMHO "history of orders" is not clear.

* @brief Get history of orders with b:a pair
* @param a asset in pair
* @param b asset in pair
* @param limit Maximum number of orders to retrieve

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

I feel "number of orders" here is a bit weird.

vector<order_history_object> get_fill_order_history( asset_id_type a, asset_id_type b, uint32_t limit )const;

/**
* @brief Gets accumulated market history with b:a pair

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

I'm not sure if this description is appropriate, but I think we should at least mention OHLC. Actually, I think we can check other exchanges' API document and write ours accordingly.

* @param a asset in pair
* @param b asset in pair
* @param bucket_seconds Represents the length in seconds of bucket
* @param start Time in seconds from where buckets begin

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

Why it's "Time in seconds"?

* @param bucket_seconds Represents the length in seconds of bucket
* @param start Time in seconds from where buckets begin
* @param end Time in seconds where buckets stop
* @return A history in market with b:a pair (maximum markets to retrieve is less than 200),

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

Why "maximum markets"?

* @param start Time in seconds from where buckets begin
* @param end Time in seconds where buckets stop
* @return A history in market with b:a pair (maximum markets to retrieve is less than 200),
* where a <= b, otherwise a and b will be swaped

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

IMHO mentioning the swap here doesn't make sense.

* @brief Gets accumulated market history with b:a pair
* @param a asset in pair
* @param b asset in pair
* @param bucket_seconds Represents the length in seconds of bucket

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

I don't know whether "bucket" is the most commonly used word for this purpose, although it's in our code.

/**
* @brief Get tracked buckets.
* Track market history by grouping orders into buckets of equal size measured in seconds
* specified as a JSON array of numbers.

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

This description starts with a verb, thus IMHO is not appropriate for this API. It seems it's copied from the plugin startup options.

/**
* @brief Generates a pedersen commitment: *commit = blind * G + value * G2.
* The commitment is 33 bytes, the blinding factor is 32 bytes.
* (For more information about pederson commitment check please next url

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

"check please" -> "please check"?

* a "range proof" must be supplied to prove that none of the outputs commit to a negative value.
* The range proofs are computed by library functions in secp256k1-zkp,
* which provides a very flexible and capable range proof library,
* in which a balance can be struck between amount of privacy and compactness of proof.

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 11, 2018

Member

I guess this is more a main description rather than a "brief", because it's long. In the meanwhile there is no main description section here.

@@ -185,11 +185,11 @@ namespace graphene { namespace app {
unsigned limit = 100,
uint32_t start = 0) const;
/**
* @brief Get list of orders with b:a pair
* @brief Get asset pairs with b:a pair

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 12, 2018

Member

"asset pairs" doesn't make sense. Please talk to traders to see what should be put here.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Sep 12, 2018

Author Member

OK. Sure. Thanks !

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2018

Jacob Walrus, [14.09.18 06:23]
[In reply to Valera Cogut]
"Fill order history" would probably sound better and be more obvious to the layperson what you are talking about if you simply used "market history". "Asset pairs" is pretty good as is but "trading pairs" might be better. "Order book" would make most sense to traders I think over "list of orders". Did that help answer your questions?

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2018

should I use "market history" instead of "asset pairs" ? or "trading pairs" ? or "order book" ?
Guys suggest please what do you think is more better suited for next API call in your opinion.

         /**
          * @brief Get asset pairs with b:a pair
          * @param a asset in pair
          * @param b asset in pair
          * @param limit Maximum count of asset pairs to retrieve
          * @return Asset pairs with b:a pair
          */
         vector<order_history_object> get_fill_order_history( asset_id_type a, asset_id_type b, uint32_t limit )const;

Thanks !

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2018

IMHO for me as I understood correctly from Jacob Warlus answer the most makes sense here "market history"

@abitmore

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

@cogutvalera apparently you've learnt some. Please talk more with your friend.

vector<bucket_object> get_market_history( asset_id_type a, asset_id_type b, uint32_t bucket_seconds,
fc::time_point_sec start, fc::time_point_sec end )const;

/**
* @brief Get list of seconds for grouping asset pairs in OHLC https://en.wikipedia.org/wiki/Open-high-low-close_chart

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 14, 2018

Member

"asset pairs" is not suitable here.

In addition, should tell the user where did this list come from.

* Thus supporting OHLC https://www.investopedia.com/terms/o/ohlcchart.asp
* @param a asset in pair
* @param b asset in pair
* @param bucket_seconds Represents the length in seconds of bucket

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 14, 2018

Member

Please note that bucket_seconds here need to be within result of get_market_history_buckets() API, otherwise no data will be returned.

* @param b asset in pair
* @param bucket_seconds Represents the length in seconds of bucket
* @param start Time point in seconds from where buckets begin, E.G. "2018-09-12T18:00:00"
* @param end Time point in seconds where buckets stop, E.G. "2018-09-12T18:00:55"

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 14, 2018

Member

I think your example here is a bit misleading or confusing. The difference of them is 55 seconds. What will return if the bucket_seconds passed in is greater than 55 seconds, for example, 1 day? Does start or end need to be "divisible" by bucket_seconds?

IMHO, a better example would be in the main description section, but not after individual parameters, because valid data range of parameters are affected by each other. In short, use most common scenario or most common input data set to make example.


/**
* @brief Get accumulated market history with b:a pair
* Thus supporting OHLC https://www.investopedia.com/terms/o/ohlcchart.asp

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 14, 2018

Member

IMHO the link is not needed here. Traders know what's OHLC.

cogutvalera added some commits Sep 17, 2018

@@ -185,30 +185,31 @@ namespace graphene { namespace app {
unsigned limit = 100,
uint32_t start = 0) const;
/**
* @brief Get asset pairs with b:a pair
* @brief Get order book with b:a pair

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 17, 2018

Member

Absolutely wrong.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Sep 18, 2018

Author Member

What is better in your opinion ? Because trader told me this variant with "order book" is better, but it isn't. Can you please assist me just a bit ?

Thanks !

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 18, 2018

Member

An API querying for history won't return an order book, that's why.
Please try understand the API, then explain it to the trader and get feedback. Modifying code cluelessly is a waste of time.

Sure I can give my opinion or even write the document by myself, however, since you asked to work on this issue, you should try to get it done by yourself.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Sep 19, 2018

Author Member

I've changed "order book" to "trading orders" in my last commit 1 day ago, is it wrong ?

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 19, 2018

Member

IMHO, "trading order" is a confusing phrase. Why not ask traders?

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Sep 23, 2018

Author Member

after our discussion and a lot of efforts trying to figure out the best suitable solution with @abitmore about API doc, fill_order_operation and after my code researches I've some results for your judgement:

  1. "matching orders" can be filled partially, so traders will be confused with such API doc description (@abitmore helped me here with more details & examples, thanks a lot !)
  2. this API method get_fill_order_history will return filled orders which also are confirmed by blockchain, IMHO the best solution is to use "confirmed orders", thus traders will understand completely what will be returned to them.

Thanks !

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 23, 2018

Member

I don't think "confirmed orders" is good enough.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Sep 23, 2018

Author Member

IMHO perhaps next:

  1. just simply "filled orders"
  2. or combination "filled and confirmed orders"

2nd version is more accurate and descriptive IMHO

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 24, 2018

Member

"filled orders" are very similar to "matched orders", and I've said why it's not good use them as is (note: I didn't say they can't be used in a sentence that's clearer). "confirmed" is very confusing. "filled and confirmed" doesn't make sense IMHO.

Can you reword the whole sentence? It's not only about choosing a noun or a phrase, it's the whole sentence that should be clearer. If unable to clearly describe it in 6 words or one sentence, please use more words and more sentences.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Sep 24, 2018

Author Member

ok, understood, sure. Thanks !

* @return A history in market with b:a pair (limit to retrieve is less than 200 buckets)
*/
vector<bucket_object> get_market_history( asset_id_type a, asset_id_type b, uint32_t bucket_seconds,
fc::time_point_sec start, fc::time_point_sec end )const;

/**
* @brief Get list of seconds for grouping asset pairs in OHLC https://en.wikipedia.org/wiki/Open-high-low-close_chart
* @return A list of seconds which are represented as unsigned integers
* @brief Get list of seconds for grouping a book's orders in OHLC

This comment has been minimized.

Copy link
@abitmore

abitmore Sep 17, 2018

Member

Wrong as well.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

just for the info: pushed new commit "fixed: (trading orders) -> (matching orders)"

@cogutvalera cogutvalera force-pushed the cogutvalera:valera_issue_780 branch from 0a49cfc to c6f603e Oct 5, 2018

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

changed last commit's message

@abitmore

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

I give up. @cogutvalera I think it's better to update the docs about orders as follows (please wrap long lines):

         /**
          * @brief Get details of order executions occurred most recently in a trading pair
          * @param a One asset in a trading pair
          * @param b The other asset in the trading pair
          * @param limit Maximum records to return
          * @return a list of order_history objects, in "most recent first" order
          */
         vector<order_history_object> get_fill_order_history( asset_id_type a, asset_id_type b, uint32_t limit )const;

         /**
          * @brief Get OHLCV data of a trading pair in a time range
          * @param a One asset in a trading pair
          * @param b The other asset in the trading pair
          * @param bucket_seconds Length of each time bucket in seconds. Note: it need to be within result of get_market_history_buckets() API, otherwise no data will be returned
          * @param start The start of a time range, E.G. "2018-01-01T00:00:00"
          * @param end The end of the time range
          * @return A list of OHLCV data, in "least recent first" order. If there are more than 200 records in the specified time range, the first 200 records will be returned.
          */
         vector<bucket_object> get_market_history( asset_id_type a, asset_id_type b, uint32_t bucket_seconds,
                                                   fc::time_point_sec start, fc::time_point_sec end )const;

         /**
          * @brief Get OHLCV time bucket lengths supported (configured) by this API server
          * @return A list of time bucket lengths in seconds. E.G. if the result contains a number "300", it means this API server supports OHLCV data aggregated in 5-minute buckets.
          */                                          
         flat_set<uint32_t> get_market_history_buckets()const;

References:

Improved in-code API doc for get_fill_order_history, get_market_histo…
…ry and get_market_history_buckets methods (Thanks @abitmore)
@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2018

@abitmore Thanks ! Changes are done.

Feature release (201810) automation moved this from In progress to In Testing Oct 6, 2018

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2018

Thanks !

@abitmore abitmore merged commit 85496e7 into bitshares:develop Oct 6, 2018

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature release (201810) automation moved this from In Testing to Done Oct 6, 2018

@cogutvalera cogutvalera referenced this pull request Oct 6, 2018

Closed

API documentation #780

5 of 8 tasks complete

@ryanRfox ryanRfox moved this from In Testing to Ready for Integration in Community Claims Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.