-
-
Notifications
You must be signed in to change notification settings - Fork 230
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
Add metric feedback to encodeData
#404
base: main
Are you sure you want to change the base?
Conversation
encodeData
@@ -262,18 +278,50 @@ function encodeField( | |||
|
|||
if (type === 'address') { | |||
if (typeof value === 'number') { | |||
return ['address', padStart(numberToBytes(value), 20)]; | |||
// Unnecessary casting - Request value could have been a hex string |
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.
Please note that these comments are temporary, will be deleted later on.
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.
In regards to this comment and in the Notion ## Address Type - Number to Address
I think this is confusing to me.
Request value could have been a hex string.
It seems that here, because it checks typeof value === 'number', it could not be a hex string
@@ -262,18 +278,50 @@ function encodeField( | |||
|
|||
if (type === 'address') { | |||
if (typeof value === 'number') { | |||
return ['address', padStart(numberToBytes(value), 20)]; | |||
// Unnecessary casting - Request value could have been a hex string |
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.
In regards to this comment and in the Notion ## Address Type - Number to Address
I think this is confusing to me.
Request value could have been a hex string.
It seems that here, because it checks typeof value === 'number', it could not be a hex string
return ['address', reallyStrangeAddressToBytes(value).subarray(0, 20)]; | ||
} | ||
} | ||
|
||
if (type === 'bool') { | ||
return ['bool', Boolean(value)]; | ||
// Unnecessary casting - Request value could have been a boolean |
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.
Unnecessary casting - Request value could have been a boolean
maybe I am not understanding the comment fully. It seems we expect it to be boolean but it could be another type. maybe we can mention is it unnecessary because we should expect explicit boolean types: true
and false
only. other types, even "true"
or "false"
should be rejected.
I guess if we did make this stricter, we could consider adding support by explicitly casting "true"/"false" or 1/0.
No description provided.