Skip to content
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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

OGPoyraz
Copy link
Member

No description provided.

@OGPoyraz OGPoyraz changed the title Add metric feedback to encodeData Add metric feedback to encodeData Dec 11, 2024
@@ -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
Copy link
Member Author

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.

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants