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

add asset names and amount/price calculations to additional #1351

Merged
merged 8 commits into from
Oct 3, 2018

Conversation

oxarbitrage
Copy link
Member

@oxarbitrage oxarbitrage commented Sep 30, 2018

#1295

Pull add some more data like asset_name into additional_data. Also calculate prices with precision.

All new data is added into new fields to keep current clients working.

In open-explorer.io this is done client side, problem is you have to make additional queries to the remote database to get precision, with kibana this is currently not possible, this should help with some visualizations.

more info: #1295 (comment)

@oxarbitrage oxarbitrage added this to the 201810 - Feature Release milestone Sep 30, 2018
@oxarbitrage oxarbitrage added 6 UX Impact flag identifying the User Interface (UX) 6 Plugin Impact flag identifying at least one plugin labels Sep 30, 2018
vs.fee_data.amount = o_v.fee_amount;
vs.fee_data.amount_calculated = (float)(o_v.fee_amount.value)/pow(10, o_v.fee_asset(db).precision);
Copy link
Contributor

Choose a reason for hiding this comment

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

precision is not greater than 12, can use a lookup table instead of expensive pow operation.
IMO amount_calculated is a bad name, perhaps better amount_units?
Using float is not nice but ok in this context I guess. Perhaps better use double?

(Same for the other _calculated fields of course.)

vs.fill_data.receives_amount = o_v.fill_receives_amount;
vs.fill_data.receives_amount_calculated = (float)(o_v.fill_receives_amount.value)/pow(10, o_v.fill_receives_asset_id(db).precision);

float fill_price = (float)(o_v.fill_receives_amount.value/o_v.fill_receives_asset_id(db).precision) /
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, must use pow(10,precision) (or lookup table like above) instead of just precision.

@@ -72,6 +73,7 @@ class elasticsearch_plugin_impl
std::string bulk_line;
std::string index_name;
bool is_sync = false;
map <uint8_t , uint64_t > lookup_pow_table;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use a simple static const array with static initialization instead of a map? Would be much simpler IMO.
Also, use double values instead of uint64_t - they're always converted to double anyway, and it even saves you some casting when using the values.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, i agree and changed. cant make it static as a member of elasticsearch_plugin_impl. maybe somewhere else, however not sure where.

1000000000,
10000000000,
100000000000,
1000000000000
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, not sure if the last three require LL modifier?

Copy link
Member

@abitmore abitmore Oct 2, 2018

Choose a reason for hiding this comment

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

FYI there is already a lookup table exists in asset.hpp. No need to reinvent the wheel.

asset::scaled_precision(n) returns 10^n.

static share_type scaled_precision( uint8_t precision )
{
FC_ASSERT( precision < 19 );
return scaled_precision_lut[ precision ];
}

Copy link
Member Author

Choose a reason for hiding this comment

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

nice one @abitmore , using that now :)

vs.fee_data.amount = o_v.fee_amount;
vs.fee_data.amount_units = (double)(o_v.fee_amount.value)/lookup_pow_table[o_v.fee_asset(db).precision];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove all (double) casts here

Copy link
Member Author

Choose a reason for hiding this comment

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

had to keep (double) cast in the fill_price_units when using the lookup table integrated in the codebase or the program will segfault with SIGFPE, Arithmetic exception.

@pmconrad
Copy link
Contributor

pmconrad commented Oct 3, 2018

Good idea to use asset::scaled_precision, but it returns a share_type. That means you must cast to double before dividing, or you lose the decimals.

@@ -235,12 +235,12 @@ void elasticsearch_plugin_impl::doVisitor(const optional <operation_history_obje
vs.fee_data.asset = o_v.fee_asset;
vs.fee_data.asset_name = o_v.fee_asset(db).symbol;
Copy link
Contributor

Choose a reason for hiding this comment

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

You're doing two lookups on fee_asset - better to look up only once.

@@ -235,12 +235,12 @@ void elasticsearch_plugin_impl::doVisitor(const optional <operation_history_obje
vs.fee_data.asset = o_v.fee_asset;
vs.fee_data.asset_name = o_v.fee_asset(db).symbol;
vs.fee_data.amount = o_v.fee_amount;
vs.fee_data.amount_units = (o_v.fee_amount.value)/asset::scaled_precision(o_v.fee_asset(db).precision).value;
vs.fee_data.amount_units = (o_v.fee_amount.value)/(double)asset::scaled_precision(o_v.fee_asset(db).precision).value;

vs.transfer_data.asset = o_v.transfer_asset_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for transfer_asset.

@@ -249,14 +249,14 @@ void elasticsearch_plugin_impl::doVisitor(const optional <operation_history_obje
vs.fill_data.pays_asset_id = o_v.fill_pays_asset_id;
vs.fill_data.pays_asset_name = o_v.fill_pays_asset_id(db).symbol;
Copy link
Contributor

Choose a reason for hiding this comment

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

Three lookups for fill_pays_asset and fill_receives_asset each.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6 Plugin Impact flag identifying at least one plugin 6 UX Impact flag identifying the User Interface (UX)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants