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
Prevent prepareTransaction from overwriting values in txJSON #1000
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I suggest writing `instructions` rather than Instructions, to be consistent with how the other parts are written |
||
} | ||
if (txJSON.LastLedgerSequence && instructions.maxLedgerVersionOffset) { | ||
return Promise.reject(new ValidationError('`LastLedgerSequence` in txJSON and `maxLedgerVersionOffset`' + | ||
' in Instructions cannot both be set')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. `instructions` here too |
||
} | ||
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,7 +136,7 @@ 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 | ||
|
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.
nit: this only returns the first invalid error, could include all of the bad fields.
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.
👍 Thanks! I think it'll be rare for someone to use multiple bad fields and if they do, it's still OK to point them out one by one.