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

Offers schema 9 #972

Merged
merged 8 commits into from
Mar 7, 2019
Merged

Offers schema 9 #972

merged 8 commits into from
Mar 7, 2019

Conversation

bartekn
Copy link
Contributor

@bartekn bartekn commented Mar 3, 2019

Updates offer table queries to use new Core DB schema 9 (stellar/stellar-core#1957). It also fixes SignersByAddress to support NULL values (close #967).

I wasn't able to test it using the code in stellar/stellar-core#1957 because it doesn't automatically upgrade the DB schema. Will test it once it's merged. EDIT: confirmed. It wasn't upgrading because of a mistake in my config file.

Copy link
Contributor

@tomquisel tomquisel 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! There's one comment about a way to do the conversion differently, but this looks correct as is if you need to rush it in.

newOffers[i] = offer

var sellingAssetCode, sellingIssuer string
err = sellingAsset.Extract(
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems unfortunate to do this Extract to create a db2/core:Offer object, only to convert it to a horizon:Offer object later (in PopulateOffer). What about modifying the db Offer object to hold xdr.Assets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really great suggestion! Simplifies a lot (code, tests, readability), thanks!

@@ -18,15 +18,20 @@ func (q *Q) SignersByAddress(dest interface{}, addy string) error {
return q.Select(dest, sql)
}

var signersXDRString string
var signersXDRString *string
Copy link
Contributor

Choose a reason for hiding this comment

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

Good fix, although I'd split into two PRs in the future.

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 call!

@bartekn bartekn merged commit 6aaac59 into master Mar 7, 2019
@bartekn bartekn deleted the offers-schema-9 branch March 7, 2019 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Horizon 0.17.0 get failed: sql: Scan error on column index 0
2 participants