stellar / go

Stellar's public monorepo of go code
https://stellar.org/developers
Apache License 2.0
1.3k stars 499 forks source link

txnbuild: generalise "Price" field #1028

Open ire-and-curses opened 5 years ago

ire-and-curses commented 5 years ago

Currently, txnbuild.ManageOffer.Price is a string. It would be cool to support number, or numerator/denominator (JS SDK supports multiple types). Existing package amount has overlap with this and could potentially be used/extended.

poliha commented 5 years ago

Here, I propose we make ManageOffer.Price an Interface{}. This will enable it support string, float and horizon protocol Price types. We can validate the types when BuildXDR() is called and return an error appropriately.

UPDATE: This approach is a breaking change and should be implemented in the next major release.

ire-and-curses commented 5 years ago

@poliha could you expand on why this is a breaking change? Naively I imagined we could do something like this:

func parsePrice(p interface{}) Price {
    switch p := p.(type) {
    case string:
    case float64:
    ...

I guess if we store the resulting price as something other than a string (e.g. a new Price object that can handle being instantiated from any of these types) then I agree it's a breaking change. Is that what you were thinking?

poliha commented 5 years ago

@ire-and-curses Yes that is what I had in mind. Currently, users expect Price to be a string. To generalise it, I was thinking the structs will become something like this

type ManageSellOffer struct {
    Selling       Asset
    Price         interface{}
    ...
}

then we can call a function like parsePrice above when building or validating the struct fields. The type change for Price field is what i referred to as the breaking change.