Assign default value for confirmationsRequired
#1809
Conversation
Pull Request Test Coverage Report for Build 10270570133Details
💛 - Coveralls |
| }, | ||
| ], | ||
| }); | ||
| // Doesn't need to fetch the Safe for it's threshold |
There was a problem hiding this comment.
Nit: It does need to fetch the Safe for 2 of the 3 transactions, I'd adjust the comment to make it clearer, wdyt?
| const hasConfirmationsRequired = data.results.every((tx) => { | ||
| return tx.confirmationsRequired !== null; | ||
| }); | ||
| if (hasConfirmationsRequired) { | ||
| return data; | ||
| } | ||
|
|
||
| const results = await Promise.all( | ||
| data.results.map(async (tx) => { | ||
| return await this._setConfirmationsRequired(tx); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Nit: this iterates data.results twice when there is an item with confirmationsRequired === null, and also executes _setConfirmationsRequired(tx) for all the transactions in the page.
No strong requirement, but maybe we can optimize it a bit by:
.then(
async (data): Promise<Page<MultisigTransaction>> => ({
...data,
results: await Promise.all(
data.results.map(async (tx) =>
tx.confirmationsRequired !== null
? tx
: await this._setConfirmationsRequired(tx),
),
),
}),
);This would also simplify the _setConfirmationRequired implementation because the initial check can be removed (and the input type) as the function would only receive transactions affected by the bug:
And a final adjustment could be also done in the other _setConfirmationRequired call in line 705:
.then(async (tx) =>
transaction.confirmationsRequired !== null
? tx
: await this._setConfirmationsRequired(tx),
);Again, this is just a nit, feel free to keep the current implementation if you think is clearer!
There was a problem hiding this comment.
I adjusted it in all three instances in 09995bb.
Assigns default values when receiving a `null` `confirmationsRequired: - Safe threshold for queued transactions - Confirmations required (if present) or Safe threshold for historical transactions
Summary
We are occasionally seeing
confirmationsRequiredof multisig transactions returned asnullby the Transaction Service. As we expect it to be an integer, this therefore fails validation.As this causes transactions not to be returned, this implements fallback values should the value be
null:Note: the above is a known bug that should eventually be tackled on the Transaction Service, after which we intend to revert the above.
Changes