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
Conversation
1c3073d
to
2a6ebc7
Compare
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.
Just took a quick glance, only had one comment. LGTM
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] + |
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.
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.
👍
src/transaction/utils.ts
Outdated
// 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 comment
The 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
src/transaction/utils.ts
Outdated
} | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
`instructions` here too
Fix #997