Add function to validate transactions #678
Conversation
| } length differs from the expected 64 bytes for a public key.`, | ||
| ); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
This return value is not parallel.
If we return true, i think it's better to have return false
| bignum(amount).cmp(bignum(MAX_TRANSACTION_AMOUNT)) > 0 | ||
| ) { | ||
| throw new Error( | ||
| 'Transaction amount must be a string integer between 0 and 18446744073709551615', |
There was a problem hiding this comment.
It would be better to use the constant MAX_TRANSACTION_AMOUNT
| } catch (error) { | ||
| throw new Error('Transaction must include a valid signature'); | ||
| } | ||
| if (![null, undefined].includes(signSignature)) { |
There was a problem hiding this comment.
Maybe we should decide if we should allow signSignature != null
| throw new Error('Transaction must include a valid id'); | ||
| } | ||
| if (fee !== FEES[type].toString()) { | ||
| throw new Error('Type 0 transactions must have a fee of 0.1 LSK'); |
There was a problem hiding this comment.
I think this is not only Type 0.
| if (typeof asset !== 'object' || asset === null || Array.isArray(asset)) { | ||
| throw new Error('Transaction must include an asset object'); | ||
| } | ||
| if (typeof asset.data !== 'undefined' && (typeof asset.data !== 'string' || asset.data.length > MAX_TRANSACTION_DATA_BYTES)) { |
There was a problem hiding this comment.
It would be better to check the asset contents depending on the transaction type
| throw new Error('Type 0 transactions must have a fee of 0.1 LSK'); | ||
| } | ||
| try { | ||
| validateAddress(recipientId); |
There was a problem hiding this comment.
depending on type, it doesn't require recipientId
| } catch (error) { | ||
| throw new Error('Transaction must include a valid recipientId'); | ||
| } | ||
| if (![null, undefined].includes(recipientPublicKey)) { |
There was a problem hiding this comment.
we should check if we should include recipientPublicKey or not depending on transaction type
| throw new Error('Transaction type must be an integer between 0 and 5'); | ||
| } | ||
| if ( | ||
| typeof amount !== 'string' || |
There was a problem hiding this comment.
I think amount should be greater than 0 only if it's type 0
|
Closing for now. We'll revisit this for v1.1.0. |
What was the problem?
We could verify transactions via verifying the signature, but not that they had the right shape. This made it difficult if you e.g. wanted to validate an input was a valid unsigned transaction before asking the user for their passphrase to sign it.
How did I fix it?
Added a function to validate the schema of a transaction.
This is a WIP - submitting a PR now to get feedback on the general approach as I'm not 100% that it's the right one to take. An alternative would be to use something like z-schema, or to wait until we implement a type system in #417 .
How to test it?
Create some valid/invalid transactions and try validating them.
Review checklist
commit guidelines