search

OpenBazaar / openbazaar-go

OpenBazaar 2.0 Server Daemon in Go
MIT License
994 stars 283 forks source link

Improved Currency Precision #1429

Open placer14 opened 5 years ago

placer14 commented 5 years ago

Today, the network assumes a certain precision when reading integer values of currency. OpenBazaar should allow each currency to be represented as an int with varying amounts of precision.

Considerations for BigInt representation (replace instances of json.Number in the Scope below with whatever solution we pick):

Scope

Protobuf Schema

proto CurrencyDefinition {
  string      code
  json.Number divisibility
}
proto CurrencyValue {
  CurrencyDefinition currency
  json.Number        value
}

*Note: I know there is redundant data with the currency type being expressed in every place where a value is represented. This was done intentionally so that knowledge of implicit relationships between fields aren't required to make sense of the data. Denormalizing data here also doesn't hurt us as we can ensure the implicit relationships are intact more easily with tests and validations.

Other Data Models

SQLite data

API changes

The JSON representation of the pb.CurrencyValue type above should be used in place of int usages for amount. The typical transformation will look similar to the following other indicated otherwise:

A value of 0.12345678 BTC might be represented like this before:

{
    "data": "value",
    "amount": 12345678, // this key is inconsistent across calls
    "coin": "BTC", // this key is inconsistent across calls
    "otherdata": "othervalue"
}

This value would be represented in scientific notification using value for all significant digits and magnitude for the exponential. Thus 0.12345678 BTC, or 12345678e-8 BTC would be represented as:

{
    "data": "value",
    "amount": { // this key would match the replaced key in the BEFORE case
        "value": "12345678",
        "magnitude": -8,
        "currency": {
            "code": "BTC", // this key would always be `code`
            "divisibility": 8
        }
    },
    "otherdata": "othervalue"
}

wallet-interface changes

Note: Some objects (like pb.CurrencyValue and pb.CurrencyDefinition) will need to be defined within the interface's package. Recommend that ob-go abstracts the wallet-interface's version of these type with it's own to cast into as the value returns from a wallet-interface call. wallet.Type indicates a defined Type local to the package wallet.

wallet-interface dependencies

auxiliary system dependencies

amangale commented 5 years ago

Thanks for starting this @placer14!

Washington and I have been talking through this scope and this is what we want to recommend:

We're in favor of a lowest LoE change that get us what we need with minimal changes to the protobuf schema and subsequently the codebase itself. Rather than the schema you proposed, we'd like to go for a straightforward change to price amounts from uint64 (for current v4) to string (for future v5). The divisibility of each coin should be configured at the wallet interface level.

The guiding principle is : Use big.Int for computation, use string for storage/representation.

Wallet interface changes

Add Divisibility to wallet-interface#Coin struct in datastore.go

type Coin interface {
    String() string
    CurrencyCode() string
    Divisibility() int
}

func (c *CoinType) Divisibility() int {
    switch *c {
    case Ethereum:
        return 18
    case TestnetEthereum:
        return 18
    default:
        return 8
    }
}

Change Utxo#Value to be string in place of current int64. Change Txn#Value to be string in place of current int64. Change TransactionCallback#Value to be string in place of current int64. Change TransactionOutput#Value to be string in place of current int64. Change TransactionInput#Value to be string in place of current int64. Change TransactionRecord#Value to be string in place of current int64.

Current

  // EstimateSpendFee should return the anticipated fee to transfer a given amount of coins
    // out of the wallet at the provided fee level. Typically this involves building a
    // transaction with enough inputs to cover the request amount and calculating the size
    // of the transaction. It is OK, if a transaction comes in after this function is called
    // that changes the estimated fee as it's only intended to be an estimate.
    //
    // All amounts should be in the coin's base unit (for example: satoshis).
  EstimateSpendFee(amount int64, feeLevel FeeLevel) (uint64, error)

Changes to

  EstimateSpendFee(amount big.Int, feeLevel FeeLevel) (big.Int, error)

Current

  // EstimateFee should return the estimate fee that will be required to make a transaction
    // spending from the given inputs to the given outputs. FeePerByte is denominated in
    // the coin's base unit (for example: satoshis).
    EstimateFee(ins []TransactionInput, outs []TransactionOutput, feePerByte uint64) uint64

Changes to

  EstimateFee(ins []TransactionInput, outs []TransactionOutput, feePerByte big.Int) big.Int

Current

  // Spend transfers the given amount of coins (in the coin's base unit. For example: in
    // satoshis) to the given address using the provided fee level. Openbazaar-go calls
    // this method in two places. 1) When the user requests a normal transfer from their
    // wallet to another address. 2) When clicking 'pay from internal wallet' to fund
    // an order the user just placed.
    // It also includes a referenceID which basically refers to the order the spend will affect
    Spend(amount int64, addr btc.Address, feeLevel FeeLevel, referenceID string) (*chainhash.Hash, error)

Changes to

  Spend(amount big.Int, addr btc.Address, feeLevel FeeLevel, referenceID string) (*chainhash.Hash, error)

Current

  // Balance returns the confirmed and unconfirmed aggregate balance for the wallet.
    // For utxo based wallets, if a spend of confirmed coins is made, the resulting "change"
    // should be also counted as confirmed even if the spending transaction is unconfirmed.
    // The reason for this that if the spend never confirms, no coins will be lost to the wallet.
    //
    // The returned balances should be in the coin's base unit (for example: satoshis)
    Balance() (confirmed, unconfirmed int64)

Changes to

  Balance() (confirmed, unconfirmed big.Int)

Current

  // IsDust returns whether the amount passed in is considered dust by network. This
    // method is called when building payout transactions from the multisig to the various
    // participants. If the amount that is supposed to be sent to a given party is below
    // the dust threshold, openbazaar-go will not pay that party to avoid building a transaction
    // that never confirms.
    IsDust(amount int64) bool

Changes to

  IsDust(amount big.Int) bool

Current

  // GetFeePerByte returns the current fee per byte for the given fee level. There
    // are three fee levels ― priority, normal, and economic.
    //
    //The returned value should be in the coin's base unit (for example: satoshis).
    GetFeePerByte(feeLevel FeeLevel) uint64

Changes to

  GetFeePerByte(feeLevel FeeLevel) big.Int

Protobuf Changes

In moderator.proto, this L26

  message Price {
        string currencyCode = 1;
        uint64 amount       = 2; // Bitcoins must be in satoshi
    }

changes to

  message Price {
        string currencyCode = 1;
        string amount       = 2; // amount must be in satoshi/wei
    }

In contracts.proto, this L61

  message Item {
        string title               = 1;
        string description         = 2;
        string processingTime      = 3;
        uint64 price               = 4;
        ...

changes to

  message Item {
        string title               = 1;
        string description         = 2;
        string processingTime      = 3;
        string price               = 4;
        ...

this L86

  message Sku {
            repeated uint32 variantCombo = 1;
            string productID             = 2;
            int64 surcharge              = 3;
            int64 quantity               = 4; // Not saved with listing
        }

changes to

  message Sku {
            repeated uint32 variantCombo  = 1;
            string productID              = 2;
            string surcharge              = 3;
            int64 quantity                = 4; // Not saved with listing
        }

this L103

  message ShippingOption {
        string name                         = 1;
        ShippingType type                   = 2;
        repeated CountryCode regions        = 3;
        repeated Service services           = 5;

        enum ShippingType {
            LOCAL_PICKUP = 0;
            FIXED_PRICE  = 1;
        }

        message Service {
            string name                = 1;
            uint64 price               = 2;
            string estimatedDelivery   = 3;
            uint64 additionalItemPrice = 4;
        }
    }

changes to

  message ShippingOption {
        string name                         = 1;
        ShippingType type                   = 2;
        repeated CountryCode regions        = 3;
        repeated Service services           = 5;

        enum ShippingType {
            LOCAL_PICKUP = 0;
            FIXED_PRICE  = 1;
        }

        message Service {
            string name                = 1;
            string price               = 2;
            string estimatedDelivery   = 3;
            string additionalItemPrice = 4;
        }
    }

this L129

  message Coupon {
        string title = 1;
        oneof code {
            string hash         = 2;
            string discountCode = 3;
        }
        oneof discount {
            float percentDiscount = 5;
            uint64 priceDiscount  = 6;
        }
    }

changes to

  message Coupon {
        string title = 1;
        oneof code {
            string hash         = 2;
            string discountCode = 3;
        }
        oneof discount {
            float percentDiscount = 5;
            string priceDiscount  = 6;
        }
    }

this L142

  message Order {
    string refundAddress                 = 1;
    uint64 refundFee                     = 2;
    ...

changes to

  message Order {
    string refundAddress                 = 1;
    string refundFee                     = 2;
    ...

this L185

  message Payment {
        Method method       = 1;
        string moderator    = 2;
        uint64 amount       = 3; // Satoshis
        ...

changes to

  message Payment {
        Method method       = 1;
        string moderator    = 2;
        string amount       = 3; // Satoshis or Wei
        ...

this L203

  message OrderConfirmation {
    string orderID                            = 1;
    google.protobuf.Timestamp timestamp       = 2;
    // Direct payments only
    string paymentAddress                     = 3;
    uint64 requestedAmount                    = 4;

    repeated RatingSignature ratingSignatures = 5;
  }

changes to

  message OrderConfirmation {
    string orderID                            = 1;
    google.protobuf.Timestamp timestamp       = 2;
    // Direct payments only
    string paymentAddress                     = 3;
    string requestedAmount                    = 4;

    repeated RatingSignature ratingSignatures = 5;
  }

this L282

  message Payout {
        repeated BitcoinSignature sigs = 1;
        string payoutAddress           = 2;
        uint64 payoutFeePerByte        = 3;
    }

changes to

  message Payout {
        repeated BitcoinSignature sigs = 1;
        string payoutAddress           = 2;
        string payoutFeePerByte        = 3;
    }

this L329

  message DisputeResolution {
    google.protobuf.Timestamp timestamp = 1;
    string orderId                      = 2;
    string proposedBy                   = 3;
    string resolution                   = 4;
    Payout payout                       = 5;
    repeated bytes moderatorRatingSigs  = 6; // Used in ratings

    message Payout {
            repeated BitcoinSignature sigs = 1;
            repeated Outpoint inputs       = 2;
            Output buyerOutput             = 3;
            Output vendorOutput            = 4;
            Output moderatorOutput         = 5;

            message Output {
              oneof scriptOrAddress {
                string script  = 1;
                string address = 3;
              }
              uint64 amount  = 2;
            }
    }
  }

changes to

  message DisputeResolution {
    google.protobuf.Timestamp timestamp = 1;
    string orderId                      = 2;
    string proposedBy                   = 3;
    string resolution                   = 4;
    Payout payout                       = 5;
    repeated bytes moderatorRatingSigs  = 6; // Used in ratings

    message Payout {
            repeated BitcoinSignature sigs = 1;
            repeated Outpoint inputs       = 2;
            Output buyerOutput             = 3;
            Output vendorOutput            = 4;
            Output moderatorOutput         = 5;

            message Output {
              oneof scriptOrAddress {
                string script  = 1;
                string address = 3;
              }
              string amount  = 2;
            }
    }
  }

this L365

  message Refund {
    string orderID                      = 1;
    google.protobuf.Timestamp timestamp = 2;
    repeated BitcoinSignature sigs      = 3;
    TransactionInfo refundTransaction   = 4;
    string memo                         = 5;

    message TransactionInfo {
        string txid  = 1;
        uint64 value = 2;
    }
  }

changes to

  message Refund {
    string orderID                      = 1;
    google.protobuf.Timestamp timestamp = 2;
    repeated BitcoinSignature sigs      = 3;
    TransactionInfo refundTransaction   = 4;
    string memo                         = 5;

    message TransactionInfo {
        string txid  = 1;
        string value = 2;
    }
  }

In api.proto this L45

  message TransactionRecord {
    string txid                         = 1;
    int64 value                         = 2;
    uint32 confirmations                = 3;
    uint32 height                       = 4;
    google.protobuf.Timestamp timestamp = 5;
  }

changes to

  message TransactionRecord {
    string txid                         = 1;
    string value                        = 2;
    uint32 confirmations                = 3;
    uint32 height                       = 4;
    google.protobuf.Timestamp timestamp = 5;
  }

API Changes

int => string type conversions for amounts will need to be changed in several locations. Beyond that, no special additions are required to the existing responses.

The conversion from string to big.Int will happen in the core package. Functions such as core/order.goL76 Purchase should return string for amount.

Repo/Sqlite DB changes

Migrate exisiting amount fields from int to string.

Sqlite does not support modifying column type as described here: https://www.sqlite.org/lang_altertable.html

The solution given is to create a temp table with the required column changes, start a transaction, insert data from old table to temp table, drop the old table, finally rename the temp table to the old table name.

The proposed wallet.Currency seems to be an overkill for the following reasons: 1) The coin does not change for a contract 2) There are no intermediate values being stored for a coin. All the amounts are stored in the finest granularity. 3) Like the coin, there is no change in the divisibility of a coin in the contract. 4) Till the time that Openbazaar does intend to support multi coin contract with varying divisibility, the proposed solution adds a lot more complexity for little mileage.

Also, shopspring/decimal is not being considered because there is no float value storage or processing in ordering.

Overall we believe this is a much simpler and direct path forward to supporting ETH-level precision that minimises schema and codebase changes.

placer14 commented 5 years ago

After discussing the suggestion in private, @amangale is on board with the plan as originally described above. For historical purposes, my observations are outlined below:

Some thoughts after reviewing Ashwin's proposal:

(TL;DR)

1. This proposal does not address in-flight contracts and our ability to support them. Providing a proper abstraction (repo.RicardianContract) which is version-aware and can be a place to hang validation/business-logic/etc is going to be needed sooner or later. If we avoid this abstraction, we're simply sprinkling copies of this logic throughout the codebase which we'll need to later reconcile (more debt).

I would personally prefer to see:

(repo.RicardianContract).contract.BuyerPayment().Definition.Divisibility or (repo.RicardianContract).contract.BuyerPayment().Divisibility

than having this sprinkled across our codebase:

if contract.Metadata.Version > 5 {
  divisibility = wallet.GetWalletForCurrencyCode(coin).CoinDivisibility()
} else {
  divisibility = MagicDivisibilityIntThatIsNotLinkedToContractData
}

2. Considering this will be a two-step approach in which the first step allows ETH contracts containing "satoshi-wei" to be used on the network, there is no explicit divisibility data associated with the amounts to know the correct magnitude when we eventually implement full precision support in step two. Assuming we did this, in step two we'd need a new field with explicit divisibility or a different name with yet more logic in the server. It would seem that adding divisibility in the first place would be prudent.

And if we're already going down the path of including this explicit data, good engineering would be to properly represent this data instead of injecting string arguments throughout our interfaces and requiring the engineer to digest and understand the code body in order to understand what the "string" is. Beyond semantics, it's poor because there's no place to hang your behavior specific to CurrencyValues without writing and creating helpers all over your code. This is not only debt, but it's high-interest.

3. Responses for your arguments of overkill for a wallet.CurrencyValue and wallet.CurrencyDefinition type:

Till the time that Openbazaar does intend to support multi coin contract with varying divisibility, the proposed solution adds a lot more complexity for little mileage.

I'll start here as it's most important. I'm not sure what complexity we're avoiding in your proposal. The only main delta that I can see (forgive me if I'm wrong here as it was hard to compare them) is that you rejected the custom type usage in favor of a string representation throughout the app. All of the same data points are being touched to change the type to a more basic representation (to use big.Int with implicit data instead of a descriptive type). Objectively, the only added complexity I see is writing MarshalJSON for the Currency* types and accessor methods.

The coin does not change for a contract Like the coin, there is no change in the divisibility of a coin in the contract.

If you're suggesting that one expression of the coin metadata is enough for an entire contract, I disagree. When we describe a Cryptocurrency Listing, the coin can be represented as a product of purchase as well as a means of payment. This necessitates independant representation of these values. If we're expressing the same data in both places, a consistant representation of the data would be preferred.

Here's an example listing:

{
    "vendorListings": [{
        "slug": "bch-eth",
        "vendorID": {
            "peerID": "QmbweQKGWNqjoPqBjePVqnTCrFsK62oWYeVe8WeqXwwyS8",
            "handle": "",
            "pubkeys": {
                "identity": "CAESIM8hU+iL8iGmlzAIMI1OGqRMWXRwNGcbW8nXOZh0eurL",
                "bitcoin": "A/n06c7eeuBo9/mWLgXQGacFI7+gnAXBlLkquCfv0srk"
            },
            "bitcoinSig": "MEUCIQCftGYX8xF+oGEZ+scMqvEasqLvh0JQ92KO81ZNg3bfMAIgIQNOpyW7eIfR50YefdF/aj9jofL491PRgFvdYHUBT24="
        },
        "metadata": {
            "version": 4,
            "contractType": "CRYPTOCURRENCY",
            "format": "MARKET_PRICE",
            "expiry": "2037-12-31T05:00:00.000Z",
            "acceptedCurrencies": ["BCH"],
            "pricingCurrency": "",
            "language": "",
            "escrowTimeoutHours": 1,
            "coinType": "ETH",
=>          "coinDivisibility": 100000000,   // payment or inventory?
            "priceModifier": 5
        },
        "item": {
            "title": "BCH-ETH",
            "description": "Cheap cryptos!",
            "processingTime": "",
            "price": 0,
            "nsfw": false,
            "tags": [],
            "images": [{
                "filename": "786a.gif",
                "original": "zb2rhn3zyAYj17JenXKGpJPBTRn68g8PJ6ZMXWFvRr7BTLLib",
                "large": "zb2rhh2BLagyb4yG8kSCTtrChHS45JAE2rLp92Zi3RwdZKK1B",
                "medium": "zb2rhnppMGkZYp6Zg7Qf2irDH9z1ZM5jc2VcAXfy6mEnifoEy",
                "small": "zb2rhfMZFaaWZxZGvkqAPCMUbmdxNHhAaCby5XCkRrV13bew8",
                "tiny": "zb2rhfN4RQyNP6eZszvEBfwBRMZxaysoqF72MYWPKoofV5AQr"
            }],
            "categories": [],
            "grams": 0,
            "condition": "",
            "options": [],
            "skus": [{
                "productID": "",
                "surcharge": 0,
                "quantity": 0
            }]
        },
        "shippingOptions": [],
        "coupons": [],
        "moderators": [],
        "termsAndConditions": "",
        "refundPolicy": ""
    }],
    "buyerOrder": {
        "refundAddress": "qreeeu24nq4f3pc5wel4ucwaxpg3ug35lg5t33566e",
        "refundFee": 0,
        "shipping": {
            "shipTo": "",
            "address": "",
            "city": "",
            "state": "",
            "postalCode": "",
            "country": "NA",
            "addressNotes": ""
        },
        "buyerID": {
            "peerID": "Qmb8CvXLDChhU5YBApLKbs8ixfWh9X6dmPHbp6B5tjFqB1",
            "handle": "",
            "pubkeys": {
                "identity": "CAESIHh9uLUPanFQVVIFkYsypxCezy/JYAx/pqu2cjUYIDZo",
                "bitcoin": "A0/wPzw14M6i/l+INxQY3JdvVOkV2lLNev+TxVjGBxS6"
            },
            "bitcoinSig": "MEQCIEx4s0UxxMK44+6KZhJAyQE64DtH9IJg0u38bkXbvfJxAiAGy6iVzHJwaMB3Njaq73+RrQ91VQnhqyc26o4GGi9XSg=="
        },
        "timestamp": "2019-01-31T21:12:20.170965750Z",
        "items": [{
            "listingHash": "zb2rhgg9mdHRub6DaBKBTu8Akg9UYYdz4azpBpdVKKNhTqMFB",
            "quantity": 0,
=>          "quantity64": 1,        // 1 what?
=>          "preciseQuantity": "1", // future field inclusion makes these quantity fields ambiguous
            "memo": "",
            "paymentAddress": "Addresss"
        }],
        "payment": {
            "method": "ADDRESS_REQUEST",
            "moderator": "",
            "amount": 0,
            "chaincode": "",
            "address": "",
            "redeemScript": "",
            "coin": "BCH"
        },
        "ratingKeys": ["Ar1GMyRSUfS+rVZ7k8SOktFPISgU1wBu3jJkQ2Sci2eP"],
        "alternateContactInfo": "",
        "version": 2
    },
    "vendorOrderConfirmation": {
        "orderID": "QmVMf7iXQxvgbo1r6JoiT22Zyz334mV6xoQmN5bLfB5LVY",
        "timestamp": "2019-01-31T21:12:21.258215870Z",
        "paymentAddress": "qpf0sv9d5q9rurg40z3es9270j7hktv9fsncfegvlk",
        "requestedAmount": 0
    },
    "signatures": [{
        "section": "LISTING",
        "signatureBytes": "ga81iV5H6UnFQ1d8HVBjLapNCvLkgFSBd2ziNMQ2Bez7DHXcrL5+qynPeT2TbqPIUvqySAUQleNDSRTEEWRWDA=="
    }, {
        "section": "ORDER",
        "signatureBytes": "FzXD5GmxHgdzSL9FkgHeSs4BLApG3t8dOTulJEwCKEdP9tOWIZU8ZWLf5veiuw/Rn9OtSVgBAzz6pHeI8K4pCw=="
    }, {
        "section": "ORDER_CONFIRMATION",
        "signatureBytes": "m/mOeypWiz9epxwrmcENJjSVQF6ITpaGnvCCtNzF7k/YO8eTSMKr45ZqitAznhZsQbKl4O99mgAakqlODGlzAw=="
    }]
}

If you're suggesting that the coin will never change over time (as in from the time that contract is created), I disagree for two reasons. Contracts have technically infinite lifetime as new orders may be executed at any time in the future. While there's precedent for us to void old versions of contracts with new versions of the server, this has been in extreme scenarios and is avoided at all costs. (And is really just a cost of not paying the debt outlined in point (1) above.) More importantly, is the case I outlined in point (2) above. If the wallet is accepting satoshi-wei now and later actual-wei and there's no way to differentiate that in the contract, the contract suddenly becomes a lie (for the value spent) because the relationship between the contract and the server API are non-existent and there's no way to resolve that.

Not to mention the cases where a coin would hardfork to create a larger supply (particularly in the case of utility coins).

There are no intermediate values being stored for a coin. All the amounts are stored in the finest granularity.

No argument from me there. I agree less complexity is preferred over more complexity. But (2) describes why we need a more complex representation. This is an obvious technical case we need to support if we decided to take a two-step approach.

Will prepare planning out the above epic into executable steps for planning and discussion.

placer14 commented 5 years ago

Additionally, we briefly discussed the plan of attack for this large scope w/ @drwasho and @amangale. The idea was raised to begin in the ETH wallet implementation as it is the highest risk and could be nice to have the chance to abort the plan if something comes up. I agree with this and with this thought in mind, here's my thinking about how to execute the beginning:

  1. wallet interfaces for CurrencyValue and CurrencyDefinition. Prefer these types to be separate so a wallet-interface could deliver func Definition() *wallet.CurrencyDefinition in place of func CurrencyCode() string
  2. Support serializing new data into the datastore (for ETH only right now, keeping in mind how this must work for all wallet implementations)
  3. datastore migrations which support the wallet interface (stxos/txns/utxos contain balance whose type needs to be updated)
  4. Update wallet-interface to use new data types

Let's plan to talk through this scope today.