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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
May 9, 2018
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4cbce8a
Fix sum of asset in circulation in all assets endpoint
bartekn May 7, 2018
6cc71e7
Add migrations
bartekn May 8, 2018
fe74c26
Add migrations entry to latest.sql
bartekn May 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions amount/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,21 @@ func ParseInt64(v string) (int64, error) {
return i, nil
}

// IntStringToAmount converts string integer value and converts it to stellar
// "amount". In other words, it divides the given string integer value by 10^7
// and returns the string representation of that number.
// It is safe to use with values exceeding int64 limits.
func IntStringToAmount(v string) (string, error) {
r := &big.Rat{}
if _, ok := r.SetString(v); !ok {
return "", errors.Errorf("cannot parse amount: %s", v)
}

r.Quo(r, bigOne)

return r.FloatString(7), nil
}

// String returns an "amount string" from the provided raw xdr.Int64 value `v`.
func String(v xdr.Int64) string {
return StringFromInt64(int64(v))
Expand Down
43 changes: 43 additions & 0 deletions amount/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package amount_test

import (
"fmt"
"testing"

"github.com/stellar/go/amount"
Expand Down Expand Up @@ -58,3 +59,45 @@ func TestString(t *testing.T) {
}
}
}

func TestIntStringToAmount(t *testing.T) {
var testCases = []struct {
Output string
Input string
Valid bool
}{
{"100.0000000", "1000000000", true},
{"-100.0000000", "-1000000000", true},
{"100.0000001", "1000000001", true},
{"123.0000001", "1230000001", true},
{"922337203685.4775807", "9223372036854775807", true},
{"922337203685.4775808", "9223372036854775808", true},
{"92233.7203686", "922337203686", true},
{"-922337203685.4775808", "-9223372036854775808", true},
{"-922337203685.4775809", "-9223372036854775809", true},
{"-92233.7203686", "-922337203686", true},
{"1000000000000.0000000", "10000000000000000000", true},
{"0.0000000", "0", true},
{"", "nan", false},
}

for _, tc := range testCases {
t.Run(fmt.Sprintf("%s to %s (valid = %t)", tc.Input, tc.Output, tc.Valid), func(t *testing.T) {
o, err := amount.IntStringToAmount(tc.Input)

if !tc.Valid && err == nil {
t.Errorf("expected err for input %s (output: %s)", tc.Input, tc.Output)
return
}
if tc.Valid && err != nil {
t.Errorf("couldn't parse %s: %v", tc.Input, err)
return
}

if o != tc.Output {
t.Errorf("%s converted to %s, not %s", tc.Input, o, tc.Output)
}
})
}

}
2 changes: 1 addition & 1 deletion services/horizon/internal/db2/assets/asset_stat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ type AssetStatsR struct {
Type string `db:"asset_type"`
Code string `db:"asset_code"`
Issuer string `db:"asset_issuer"`
Amount int64 `db:"amount"`
Amount string `db:"amount"`
NumAccounts int32 `db:"num_accounts"`
Flags int8 `db:"flags"`
Toml string `db:"toml"`
Expand Down
6 changes: 3 additions & 3 deletions services/horizon/internal/db2/assets/asset_stat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ func TestAssetsStatsQExec(t *testing.T) {
Type: "credit_alphanum4",
Code: "BTC",
Issuer: "GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
Amount: 1009876000,
Amount: "1009876000",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's converted in resources.AssetStat.Populate.

NumAccounts: 1,
Flags: 1,
Toml: "https://test.com/.well-known/stellar.toml",
Expand All @@ -26,7 +26,7 @@ func TestAssetsStatsQExec(t *testing.T) {
Type: "credit_alphanum4",
Code: "SCOT",
Issuer: "GCXKG6RN4ONIEPCMNFB732A436Z5PNDSRLGWK7GBLCMQLIFO4S7EYWVU",
Amount: 10000000000,
Amount: "10000000000",
NumAccounts: 1,
Flags: 2,
Toml: "",
Expand All @@ -37,7 +37,7 @@ func TestAssetsStatsQExec(t *testing.T) {
Type: "credit_alphanum4",
Code: "USD",
Issuer: "GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
Amount: 3000010434000,
Amount: "3000010434000",
NumAccounts: 2,
Flags: 1,
Toml: "https://test.com/.well-known/stellar.toml",
Expand Down
6 changes: 3 additions & 3 deletions services/horizon/internal/db2/core/trustline.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,16 @@ func (q *Q) BalancesForAsset(
assetType int32,
assetCode string,
assetIssuer string,
) (int32, int64, error) {
) (int32, string, error) {
sql := selectBalances.Where(sq.Eq{
"assettype": assetType,
"assetcode": assetCode,
"issuer": assetIssuer,
"flags": 1,
})
result := struct {
Count int32 `db:"count"`
Sum int64 `db:"sum"`
Count int32 `db:"count"`
Sum string `db:"sum"`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}{}
err := q.Get(&result, sql)
return result.Count, result.Sum, err
Expand Down
2 changes: 1 addition & 1 deletion services/horizon/internal/db2/history/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

NumAccounts int32 `db:"num_accounts"`
Flags int8 `db:"flags"`
Toml string `db:"toml"`
Expand Down
51 changes: 37 additions & 14 deletions services/horizon/internal/db2/schema/bindata.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion services/horizon/internal/db2/schema/latest.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@bartekn bartekn May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nullstyle nullstyle May 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

num_accounts integer NOT NULL,
flags smallint NOT NULL,
toml character varying(64) NOT NULL
Expand Down
7 changes: 7 additions & 0 deletions services/horizon/internal/db2/schema/migrations/12_asset_stats_amount_string.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-- +migrate Up
ALTER TABLE asset_stats
ALTER COLUMN amount SET DATA TYPE character varying;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


-- +migrate Down
ALTER TABLE asset_stats
ALTER COLUMN amount SET DATA TYPE bigint USING amount::bigint;
2 changes: 1 addition & 1 deletion services/horizon/internal/ingest/asset_stat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ func computeAssetStat(is *Session, asset *xdr.Asset) *history.AssetStat {
}

// statTrustlinesInfo fetches all the stats from the trustlines table
func statTrustlinesInfo(coreQ *core.Q, assetType xdr.AssetType, assetCode string, assetIssuer string) (int32, int64, error) {
func statTrustlinesInfo(coreQ *core.Q, assetType xdr.AssetType, assetCode string, assetIssuer string) (int32, string, error) {
return coreQ.BalancesForAsset(int32(assetType), assetCode, assetIssuer)
}

Expand Down
22 changes: 11 additions & 11 deletions services/horizon/internal/ingest/asset_stat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func TestStatTrustlinesInfo(t *testing.T) {
assetCode string
assetIssuer string
wantNumAccounts int32
wantAmount int64
wantAmount string
}

testCases := []struct {
Expand All @@ -33,7 +33,7 @@ func TestStatTrustlinesInfo(t *testing.T) {
"USD",
"GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
1,
0,
"0",
}},
}, {
"asset_stat_trustlines_2",
Expand All @@ -42,7 +42,7 @@ func TestStatTrustlinesInfo(t *testing.T) {
"USD",
"GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
1,
0,
"0",
}},
}, {
"asset_stat_trustlines_3",
Expand All @@ -51,13 +51,13 @@ func TestStatTrustlinesInfo(t *testing.T) {
"USD1",
"GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
1,
0,
"0",
}, {
xdr.AssetTypeAssetTypeCreditAlphanum4,
"USD2",
"GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
1,
0,
"0",
}},
}, {
"asset_stat_trustlines_4",
Expand All @@ -66,13 +66,13 @@ func TestStatTrustlinesInfo(t *testing.T) {
"USD",
"GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
1,
0,
"0",
}, {
xdr.AssetTypeAssetTypeCreditAlphanum4,
"USD",
"GCXKG6RN4ONIEPCMNFB732A436Z5PNDSRLGWK7GBLCMQLIFO4S7EYWVU",
1,
0,
"0",
}},
}, {
"asset_stat_trustlines_5",
Expand All @@ -81,7 +81,7 @@ func TestStatTrustlinesInfo(t *testing.T) {
"USD",
"GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
1,
0,
"0",
}},
}, {
"asset_stat_trustlines_6",
Expand All @@ -90,7 +90,7 @@ func TestStatTrustlinesInfo(t *testing.T) {
"USD",
"GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
1,
1012345000,
"1012345000",
}},
}, {
"asset_stat_trustlines_7",
Expand All @@ -99,7 +99,7 @@ func TestStatTrustlinesInfo(t *testing.T) {
"USD",
"GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
2,
1012345000,
"1012345000",
}},
}, {
"allow_trust",
Expand All @@ -108,7 +108,7 @@ func TestStatTrustlinesInfo(t *testing.T) {
"USD",
"GC23QF2HUE52AMXUFUH3AYJAXXGXXV2VHXYYR6EYXETPKDXZSAW67XO4",
1, // assets with the auth_required flag should only be counted for authorized accounts
0,
"0",
}},
},
}
Expand Down
5 changes: 4 additions & 1 deletion services/horizon/internal/resource/asset_stat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ func (res *AssetStat) Populate(
res.Asset.Type = row.Type
res.Asset.Code = row.Code
res.Asset.Issuer = row.Issuer
res.Amount = amount.StringFromInt64(row.Amount)
res.Amount, err = amount.IntStringToAmount(row.Amount)
if err != nil {
return err
}
res.NumAccounts = row.NumAccounts
res.Flags = AccountFlags{
(row.Flags & int8(xdr.AccountFlagsAuthRequiredFlag)) != 0,
Expand Down