-
Notifications
You must be signed in to change notification settings - Fork 499
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
Offers schema 9 #972
Conversation
There was a problem hiding this 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( |
There was a problem hiding this comment.
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.Asset
s?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call!
Updates
offer
table queries to use new Core DB schema 9 (stellar/stellar-core#1957). It also fixesSignersByAddress
to supportNULL
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.