fix(bug): toTxMethod sanitizing #183
Conversation
| @@ -43,7 +43,7 @@ export function toTxMethod(registry: TypeRegistry, method: Call): TxMethod { | |||
|
|
|||
| // Sanity check to check that `jsonArg` is a number, and if it is | |||
| // to change it to a string | |||
| if (!Number.isNaN(jsonArg)) { | |||
| if (!Number.isNaN(jsonArg) && typeof jsonArg === 'number') { | |||
There was a problem hiding this comment.
This line looks a wee bit sketchy to me.
I assume that the first part of the ternary is guaranteed to return a number, i.e. if codec is an AbstractInt I assume that the conversion to string is fine and need no further checking.
If that is true, then this line is here to guard against the second part, (codec as BN).toJSON()?
But then why not do this checking here in that else branch?
if codec instanceof AbstractInt {
codec.toString(RADIX_PARAM)
} else {
(codec as BN).toJSON();
// check here
}
Not very familiar with BN but it seems odd that a .toJSON() call could return something that isn't a number. What exactly could codec contain that makes this necessary?
There was a problem hiding this comment.
@dvdplm BN.toJSON() just returns a string representation of the number.
There was a problem hiding this comment.
@dvdplm I think the concern here is 100% valid, and I will clean up the code to make it more readable, and cleaner.
The issue with (codec as BN).toJSON() is that it is aactually technically incorrect as the codec arg will not always be guaranteed to be a BN, but may be an object with the raw type of:
{"_enum":{"Id":"AccountId","Index":"Compact<AccountIndex>","Raw":"Bytes","Address32":"H256","Address20":"H160"}}
as well as any other valid codec type.
or it could also very well be a BN.
Co-authored-by: David <dvdplm@gmail.com>
closes: #180
rel: #126
This fixes a bug introduced in #178 where an increased effort to sanitize numbers via
toTxMethodwas given, this makes sure we check the type of the primitive value as well as!Number.isNaNto ensure any integer that makes it past the first initial sanitization is converted to a string.I also added a test suite for
toTxMethod(long over due).I can also confirm with the given test the older logic was failing and is now passing.