Prevent prepareTransaction from overwriting values in txJSON (#1000)

Fix #997

HISTORY.md

@@ -1,5 +1,9 @@
 # ripple-lib Release History
 
+## UNRELEASED
+
+* Prevent `prepareTransaction` from overwriting `Fee` and/or `LastLedgerSequence` (#997)
+
 ## 1.2.1 (2019-03-23)
 
 * Update `ripple-binary-codec` to 0.2.1 to support `tecKILLED`

src/common/schemas/input/get-transaction.json

@@ -10,14 +10,14 @@
       "properties": {
         "minLedgerVersion": {
           "$ref": "ledgerVersion",
-          "description": "The lowest ledger version to search."
+          "description": "The lowest ledger version to search. This must be an integer greater than 0, or one of the following strings: 'validated', 'closed', 'current'."
         },
         "maxLedgerVersion": {
           "$ref": "ledgerVersion",
-          "description": "The highest ledger version to search"
+          "description": "The highest ledger version to search. This must be an integer greater than 0, or one of the following strings: 'validated', 'closed', 'current'."
         },
         "includeRawTransaction": {
-          "description": "Include raw transaction data. For advanced users; exercise caution when interpreting this data. "
+          "description": "Include raw transaction data. For advanced users; exercise caution when interpreting this data."
         }
       },
       "additionalProperties": false

src/common/schemas/objects/instructions.json

@@ -18,7 +18,7 @@
       "$ref": "value"
     },
     "maxLedgerVersion": {
-      "description": "The highest ledger version that the transaction can be included in. If this option and `maxLedgerVersionOffset` are both omitted, the `maxLedgerVersion` option will default to 3 greater than the current validated ledger version (equivalent to `maxLedgerVersionOffset=3`). Use `null` to not set a maximum ledger version.",
+      "description": "The highest ledger version that the transaction can be included in. If this option and `maxLedgerVersionOffset` are both omitted, the `maxLedgerVersion` option will default to 3 greater than the current validated ledger version (equivalent to `maxLedgerVersionOffset=3`). Use `null` to not set a maximum ledger version. If not null, this must be an integer greater than 0, or one of the following strings: 'validated', 'closed', 'current'.",
       "oneOf": [
         {"$ref": "ledgerVersion"},
         {"type": "null"}

src/common/schemas/output/prepare.json

@@ -14,7 +14,7 @@
       "properties": {
         "fee": {
           "$ref": "value",
-          "description": "An exact fee to pay for the transaction. See [Transaction Fees](#transaction-fees) for more information."
+          "description": "The fee to pay for the transaction. See [Transaction Fees](#transaction-fees) for more information. For multi-signed transactions, this fee will be multiplied by (N+1), where N is the number of signatures you plan to provide."
         },
         "sequence": {
           "$ref": "sequence",
@@ -25,7 +25,7 @@
             {"$ref": "ledgerVersion"},
             {"type": "null"}
           ],
-          "description": "The highest ledger version that the transaction can be included in. Set to `null` if there is no maximum."
+          "description": "The highest ledger version that the transaction can be included in. Set to `null` if there is no maximum. If not null, this must be an integer greater than 0, or one of the following strings: 'validated', 'closed', 'current'."
         }
       },
       "additionalProperties": false,

src/transaction/utils.ts

@@ -51,11 +51,32 @@ function prepareTransaction(txJSON: TransactionJSON, api: RippleAPI,
 ): Promise<Prepare> {
   common.validate.instructions(instructions)
   common.validate.tx_json(txJSON)
+  const disallowedFieldsInTxJSON = ['maxLedgerVersion', 'maxLedgerVersionOffset', 'fee', 'sequence']
+  const badFields = disallowedFieldsInTxJSON.filter(field => txJSON[field])
+  if (badFields.length) {
+    return Promise.reject(new ValidationError('txJSON additionalProperty "' + badFields[0] +
+      '" exists in instance when not allowed'))
+  }
 
   const account = txJSON.Account
   setCanonicalFlag(txJSON)
 
-  function prepareMaxLedgerVersion(): Promise<object> {
+  function prepareMaxLedgerVersion(): Promise<TransactionJSON> {
+    // Up to one of the following is allowed:
+    //   txJSON.LastLedgerSequence
+    //   instructions.maxLedgerVersion
+    //   instructions.maxLedgerVersionOffset
+    if (txJSON.LastLedgerSequence && instructions.maxLedgerVersion) {
+      return Promise.reject(new ValidationError('`LastLedgerSequence` in txJSON and `maxLedgerVersion`' +
+        ' in `instructions` cannot both be set'))
+    }
+    if (txJSON.LastLedgerSequence && instructions.maxLedgerVersionOffset) {
+      return Promise.reject(new ValidationError('`LastLedgerSequence` in txJSON and `maxLedgerVersionOffset`' +
+        ' in `instructions` cannot both be set'))
+    }
+    if (txJSON.LastLedgerSequence) {
+      return Promise.resolve(txJSON)
+    }
     if (instructions.maxLedgerVersion !== undefined) {
       if (instructions.maxLedgerVersion !== null) {
         txJSON.LastLedgerSequence = instructions.maxLedgerVersion
@@ -70,16 +91,27 @@ function prepareTransaction(txJSON: TransactionJSON, api: RippleAPI,
     })
   }
 
-  function prepareFee(): Promise<object> {
+  function prepareFee(): Promise<TransactionJSON> {
+    // instructions.fee is scaled (for multi-signed transactions) while txJSON.Fee is not.
+    // Due to this difference, we do NOT allow both to be set, as the behavior would be complex and
+    // potentially ambiguous.
+    // Furthermore, txJSON.Fee is in drops while instructions.fee is in XRP, which would just add to
+    // the confusion. It is simpler to require that only one is used.
+    if (txJSON.Fee && instructions.fee) {
+      return Promise.reject(new ValidationError('`Fee` in txJSON and `fee` in `instructions` cannot both be set'))
+    }
+    if (txJSON.Fee) {
+      // txJSON.Fee is set. Use this value and do not scale it.
+      return Promise.resolve(txJSON)
+    }
     const multiplier = instructions.signersCount === undefined ? 1 :
       instructions.signersCount + 1
     if (instructions.fee !== undefined) {
       const fee = new BigNumber(instructions.fee)
       if (fee.greaterThan(api._maxFeeXRP)) {
-        const errorMessage = `Fee of ${fee.toString(10)} XRP exceeds ` +
+        return Promise.reject(new ValidationError(`Fee of ${fee.toString(10)} XRP exceeds ` +
           `max of ${api._maxFeeXRP} XRP. To use this fee, increase ` +
-          '`maxFeeXRP` in the RippleAPI constructor.'
-        throw new ValidationError(errorMessage)
+          '`maxFeeXRP` in the RippleAPI constructor.'))
       }
       txJSON.Fee = scaleValue(common.xrpToDrops(instructions.fee), multiplier)
       return Promise.resolve(txJSON)
@@ -104,14 +136,14 @@ function prepareTransaction(txJSON: TransactionJSON, api: RippleAPI,
     })
   }
 
-  async function prepareSequence(): Promise<object> {
+  async function prepareSequence(): Promise<TransactionJSON> {
     if (instructions.sequence !== undefined) {
       if (txJSON.Sequence === undefined || instructions.sequence === txJSON.Sequence) {
         txJSON.Sequence = instructions.sequence
         return Promise.resolve(txJSON)
       } else {
         // Both txJSON.Sequence and instructions.sequence are defined, and they are NOT equal
-        return Promise.reject(new ValidationError('`Sequence` in txJSON must match `sequence` in Instructions'))
+        return Promise.reject(new ValidationError('`Sequence` in txJSON must match `sequence` in `instructions`'))
       }
     }
     if (txJSON.Sequence !== undefined) {