Fix isSigned returning true for unsigned txs #1042
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
| data: data ?? emptyBuffer, | ||
| v: !v?.equals(emptyBuffer) ? new BN(v) : undefined, | ||
| r: !r?.equals(emptyBuffer) ? new BN(r) : undefined, | ||
| s: !s?.equals(emptyBuffer) ? new BN(s) : undefined, |
There was a problem hiding this comment.
I find these lines somewhat hard to read, replicated on REPL to trust them. No blocker though.
There was a problem hiding this comment.
Is the question mark necessary? What does it do? The ! implies that it is defined, so I don't see what this ? should do?
There was a problem hiding this comment.
I agree that this is not the easiest to read, but I found it the best option for this particular use. I am happy to change if there is a better suggestion.
Is the question mark necessary? What does it do? The
!implies that it is defined, so I don't see what this?should do?
The ! negates the equals expression to ensure that it doesn't match emptyBuffer. It would imply that it is defined if it was in the position of the ?. The ? is optional chaining so that if it is undefined, it will revert to the false clause of the ternary and output undefined.
If ternary expressions could accept && clauses, then It could be written as: v && !v.equals(emptyBuffer) ? new BN(v) : undefined
There was a problem hiding this comment.
Hi, the bug is actually not fixed (I should have noticed this earlier especially after your comment) - indeed, if v does not exist, the ? operator makes it false, but it is then negated - so it is true. There's always a BN created.
let r = undefined
console.log(!r?.equals(emptyBuffer) ? new BN(r) : undefined)
Prints <BN 0>
There was a problem hiding this comment.
hmmm oh man I am just testing this again, I have to check why the new tests I added in this PR are passing okay then...
I am also wondering why in the comment above I was confused because ternary expression do accept && clauses (must have been confused with something else), so this example code I wrote might be more reasonable to read:
v && !v.equals(emptyBuffer) ? new BN(v) : undefined
I saw you updated in your PR to:
r !== undefined && !r?.equals(emptyBuffer) ? new BN(r) : undefined,
but in that case the optional chaining ? is not needed anymore (since it's already being checked for undefined) so this is a lot more understandable:
r !== undefined && !r.equals(emptyBuffer) ? new BN(r) : undefined,
There was a problem hiding this comment.
Ah yes good point! I still find it very hard to read though, so if there's a better alternative that'd be great :)
There was a problem hiding this comment.
I opened a PR to fix this: #1077
This PR fixes
tx.isSigned()as reported in #1041. It ensures thatv,r,saren't initialized in the constructor if undefined.In writing some extra test cases, I also found the same bug in
fromValuesArraythat would initializev,r,stoBN(0)if missing, so that is also fixed.