-
Notifications
You must be signed in to change notification settings - Fork 5k
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 string event integration tests #3497
Conversation
55bc05e
to
43ef593
Compare
} | ||
|
||
function firesIllegalUtf8StringEvent() public { | ||
emit StringEvent('�������'); |
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.
Try emit StringEvent(string(0x9f90e274657374))
.
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.
solc is not letting me do this (have tried a few variants). Keep getting something along these lines:
TypeError: Explicit type conversion not allowed from "int_const 44913823586808692"
to "string memory".
emit StringEvent(string(0x9f90e274657374));
^----------------------^
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.
My bad, I don't usually use bytes literals. Try emit StringEvent(string(hex"9f90e274657374"))
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.
@cag Ah that works but it's detecting that it's invalid. Also tried w/ 0.4.26 and it's the same...
TypeError: Explicit type conversion not allowed from "literal_string (contains invalid UTF-8 sequence
at position 0)" to "string memory".
emit StringEvent(string(hex"9f90e274657374"));
^-------------------------^
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.
We don't have to use this kind of test fwiw, we could also try to trigger it using a raw log, as shown here.
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.
Also tried using a raw log style test without success (e.g failure)
43ef593
to
a298c9e
Compare
3655531
to
0c32153
Compare
More utf-8 stuff: #3480 |
162b17c
to
59cd8e8
Compare
really nice to have these extra tests 👍 were you able to try decoding a malformed utf-8 string from a contract to see how web3 would react? @ricmoo mentioned flipping a top bit of any byte:
If ether’s default behavior is to error in this case (but also provides a way to recover the string) I wonder if it would be of benefit to the community for web3.js to have a differing default behavior. Since we have relaxed strictness around some encoding formats, one mentality could be for web3 try to “fail as little as possible” in decoding so if a user is interacting with a smart contract he doesn’t own and someone has stored or emits bad string data, the code tries to continue to run instead of crashing (if not caught). We could introduce a param to enforce strict validation on decoding strings that is default false. Easier said than done, but just an idea. |
I have tried to do this here, but have been unsure about how to malform the strings. The recipe you suggest is great, will see if I can do that, thanks! |
Took another look at this and remembered that I got the inputs for these from an w3.org test case set for malformed utf-8. (Apologies - I should have documented this.) The new tests use case
This seems quite similar to what @ricmoo suggests. Not certain how to proceed here. Am going to update the file with a note on the test case source for now... |
This needs a rebase, there are irrelevant changes in the diff.... |
04228c1
to
e2bc557
Compare
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.
LGTM, thanks for investigating and adding the additional tests!
Description
Adds integration tests for illegal utf-8 string and wide unicode (emoji) event decoding.
Type of change
Checklist: