Fix sum of asset in circulation in all assets endpoint #436

bartekn · 2018-05-07T17:47:08Z

Currently we hold sum of asset in circulation in int64 variable. Given
that issuer can send MAX_INT64 of an asset to each of it's trustors
it's very easy to overflow the type.

This commit changes the type a variable that holds DB result for a query
to string and adds a new function in amount package to convert
string integer to Stellar amount.

Currently we hold sum of asset in circulation in `int64` variable. Given that issuer can send `MAX_INT64` of an asset to each of it's trustees it's very easy to overflow the type. This commit changes the type a variable that holds DB result for a query to `string` and adds a new function in `amount` package to convert string integer to Stellar amount.

nikhilsaraf · 2018-05-07T20:56:38Z

@bartekn how does this affect data that is already in the table --do we need an upgrade script or an compatibility code?

bartekn · 2018-05-08T07:39:58Z

We probably need to reingest the history.
EDIT Thanks for explaining this during a call @nikhilsaraf, you're right, we don't need to reingest history, a simple script that updates the data should be enough.

nikhilsaraf · 2018-05-08T18:34:01Z

services/horizon/internal/db2/assets/asset_stat_test.go

@@ -15,7 +15,7 @@ func TestAssetsStatsQExec(t *testing.T) {
 		Type:        "credit_alphanum4",
 		Code:        "BTC",
 		Issuer:      "GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
-		Amount:      1009876000,
+		Amount:      "1009876000",


@bartekn should this be 1009876000 * 10^7 = "10098760000000000"?

It's converted in resources.AssetStat.Populate.

nikhilsaraf · 2018-05-08T18:38:17Z

services/horizon/internal/db2/core/trustline.go

-		Count int32 `db:"count"`
-		Sum   int64 `db:"sum"`
+		Count int32  `db:"count"`
+		Sum   string `db:"sum"`


the query below var selectBalances = sq.Select("COUNT(*)", "COALESCE(SUM(balance), 0) as sum").From("trustlines") will also need to change since balance is an int so will the DB's SUM operation overflow and wil lthat return an int64 or a string? fyi @bartekn

Good point which also shows we need a test for this SUM(balance) larger than bigint.

OK, it seems we're good here because postgres returns numeric type for sum of bigints and balance field in core is bigint. But I will add the test anyway tomorrow.

nikhilsaraf · 2018-05-08T18:39:32Z

services/horizon/internal/db2/history/main.go

@@ -128,7 +128,7 @@ type Asset struct {
 // AssetStat is a row in the asset_stats table representing the stats per Asset
 type AssetStat struct {
 	ID          int64  `db:"id"`
-	Amount      int64  `db:"amount"`
+	Amount      string `db:"amount"`


this will need the db upgrade script like we discussed on slack -- change the row in the assetStats table to be a character varying instead of a bigint

nikhilsaraf · 2018-05-08T18:45:28Z

Added some inline comments.

Summarizing our discussion on slack, @bartekn:
General approach we discussed on slack at least for immediate fix was to convert the amount field in the db to a varchar as a db upgrade script, this will fix the value in the db so the stats table can hold the value correctly, and the code changes in the PR here will ensure that the runtime logic can support calculations that exceed the capacity of int64.

nikhilsaraf

commented on migrations

nikhilsaraf · 2018-05-08T19:09:50Z

services/horizon/internal/db2/schema/latest.sql

@@ -102,7 +102,7 @@ SET default_with_oids = false;

 CREATE TABLE asset_stats (
    id bigint NOT NULL,
-    amount bigint NOT NULL,
+    amount character varying NOT NULL,


isn't this supposed to be left as-is?? @bartekn
anyone who starts a fresh install will go through both scripts so they will end up with the correct version.

AFAIK, latest.sql should contain the schema dump after applying all migrations. From @nullstyle:

The latest schema allows a new database to go from empty to the correct schema without all of the intermediary steps. This becomes important when you hit 50+ migrations.

#86 (comment)

ok good to know. so I guess there's some logic which knows when to apply latest vs migrations

@nikhilsaraf latest.sql isused when you invoke horizon db init

ok, just for my curiosity.. do we also then run migrations after that and how would it interact in a case where the latest.sql already has the correct data-type for the column and we try and update it?

ok, just for my curiosity.. do we also then run migrations after that and how would it interact in a case where the latest.sql already has the correct data-type for the column and we try and update it?

latest.sql also adds rows to gorp_migrations table which is later used by migrations code. Thanks for reminding me that I should add a new row there, fixed in fe74c26.

nikhilsaraf · 2018-05-08T19:10:33Z

services/horizon/internal/db2/schema/migrations/12_asset_stats_amount_string.sql

@@ -0,0 +1,7 @@
+-- +migrate Up
+ALTER TABLE asset_stats
+ALTER COLUMN amount SET DATA TYPE character varying;


does this need a "using" like below?
-- also we need this to be multiplied by 10^7?

does this need a "using" like below?

I tested it, it was only required for down migration.

also we need this to be multiplied by 10^7?

No, amount in asset_stats table is in stroops. We convert it to lumens in resources.AssetStat.Populate.

nikhilsaraf · 2018-05-08T20:43:11Z

@bartekn ok sounds good. this just leaves one issue to be resolved: about the selectBalances query

nullstyle

LGTM!

bartekn · 2018-05-09T17:32:32Z

OK, on the test front I think I need to create a new DB test scenario with a few accounts with MAX_INT64 balances. To do this I need to use scc which I have no experience with. Given we want to push this update asap, I checked if everything works on my local testnet node (created issuing account and 3 holders), result:

      {
        "_links": {
          "toml": {
            "href": ""
          }
        },
        "asset_type": "credit_alphanum4",
        "asset_code": "ABC",
        "asset_issuer": "GCXECIYXCLVDSGQJJEGCPR6FJVK4DR3GF655KT7LVRPXNRSMY5YILZVO",
        "paging_token": "ABC_GCXECIYXCLVDSGQJJEGCPR6FJVK4DR3GF655KT7LVRPXNRSMY5YILZVO_credit_alphanum4",
        "amount": "2767011611055.4327421",
        "num_accounts": 3,
        "flags": {
          "auth_required": false,
          "auth_revocable": false
        }
      },

I'll add a test later.

nikhilsaraf

LGTM, thanks!

bartekn requested review from nullstyle, tomerweller and nikhilsaraf May 7, 2018 17:47

bartekn added in progress horizon labels May 7, 2018

bartekn force-pushed the fix-sum-asset-aggregations branch from 11692c4 to 4cbce8a Compare May 7, 2018 18:01

nikhilsaraf reviewed May 8, 2018

View reviewed changes

Add migrations

6cc71e7

nikhilsaraf reviewed May 8, 2018

View reviewed changes

nullstyle previously approved these changes May 9, 2018

View reviewed changes

Add migrations entry to latest.sql

fe74c26

bartekn dismissed nullstyle’s stale review via fe74c26 May 9, 2018 17:27

nikhilsaraf approved these changes May 9, 2018

View reviewed changes

bartekn merged commit 8760b02 into master May 9, 2018

bartekn deleted the fix-sum-asset-aggregations branch May 9, 2018 18:35

bartekn removed the in progress label May 9, 2018

bartekn mentioned this pull request May 16, 2018

Missing toml in asset_stats #427

Closed

bartekn mentioned this pull request Aug 22, 2018

/assets numAccounts & amount incorrect #606

Closed

bartekn mentioned this pull request Sep 25, 2019

services/horizon/internal/actions: Add /order_book ingestion endpoint #1761

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sum of asset in circulation in all assets endpoint #436

Fix sum of asset in circulation in all assets endpoint #436

bartekn commented May 7, 2018 •

edited

Loading

nikhilsaraf commented May 7, 2018

bartekn commented May 8, 2018 •

edited

Loading

nikhilsaraf May 8, 2018

bartekn May 8, 2018

nikhilsaraf May 8, 2018

bartekn May 8, 2018

nikhilsaraf May 8, 2018

bartekn May 8, 2018

nikhilsaraf May 8, 2018

bartekn May 8, 2018

nikhilsaraf commented May 8, 2018

nikhilsaraf left a comment

nikhilsaraf May 8, 2018

bartekn May 8, 2018 •

edited

Loading

nikhilsaraf May 8, 2018

nullstyle May 9, 2018 •

edited

Loading

nikhilsaraf May 9, 2018

bartekn May 9, 2018

nikhilsaraf May 8, 2018

bartekn May 8, 2018

nikhilsaraf commented May 8, 2018

nullstyle left a comment

bartekn commented May 9, 2018

nikhilsaraf left a comment

Fix sum of asset in circulation in all assets endpoint #436

Fix sum of asset in circulation in all assets endpoint #436

Conversation

bartekn commented May 7, 2018 • edited Loading

nikhilsaraf commented May 7, 2018

bartekn commented May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhilsaraf commented May 8, 2018

nikhilsaraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekn May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullstyle May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhilsaraf commented May 8, 2018

nullstyle left a comment

Choose a reason for hiding this comment

bartekn commented May 9, 2018

nikhilsaraf left a comment

Choose a reason for hiding this comment

bartekn commented May 7, 2018 •

edited

Loading

bartekn commented May 8, 2018 •

edited

Loading

bartekn May 8, 2018 •

edited

Loading

nullstyle May 9, 2018 •

edited

Loading