Bugfix/encoded bool function parameter #1350
Conversation
✅ Deploy Preview for fractal-framework-interface-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
adamgall
force-pushed
the
bugfix/encoded-bool-function-parameter
branch
from
57df12d to
9caba46
Compare
test/encodeFunction.test.ts
Outdated
| expect(encodeFunction('foo', 'string', '')).toEqual(encoded); | ||
| }); | ||
|
|
||
| test('Function encoding with [string="hello, world"', () => { |
There was a problem hiding this comment.
For this failing test I'd rather comment this out - our UX flow currently depends on separating params with commas, so in order to get this working - we need to adjust the UX flow itself. Making it look and work more like we have in ProposalTemplate definitely will give us ability to support strings with comma and pretty much any other characters
There was a problem hiding this comment.
@Da-Colon Added test.skip for those expected-to-fail test cases - feel free to remove those skips if you'll figure out proper implementation
test/encodeFunction.test.ts
Outdated
| @@ -36,3 +36,27 @@ test('Function encoding with [uint8=100]', () => { | |||
| const encoded = new utils.Interface(['function foo(uint8)']).encodeFunctionData('foo', [100]); | |||
| expect(encodeFunction('foo', 'uint8', '100')).toEqual(encoded); | |||
| }); | |||
|
|
|||
| test('Function encoding with [string="true"]', () => { | |||
There was a problem hiding this comment.
This sounds more like an edge-case though, but I guess it highlights overall flakiness and fragileness of our encodeFunction
Depends on PR #1349
Description
Introduces some code to convert string
"true"/"false"into booleantrueorfalse, when passed intoethers'encodeFunctionData, which seems to be the only way to get that function to spit out the correct encoded data.Notes
I don't love this current implementation that I just did to check whether the incoming parameter is a
"true"/"false"string, and then converts it to a boolean value. Right now that logic is entirely disconnected from the actual type that the user says those parameters should be (which is declared in thefunctionSignaturevariable).Consider a function of the following signature:
foo(string bar), and the user wants to encode that string parameter with a value of"true". With this current implementation, it'll encode it as0xf31a696900000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000000,when the encoding that the user actually wants is
0xf31a6969000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000047472756500000000000000000000000000000000000000000000000000000000This happens because if we see the string
"true"anywhere in theparameterlist, we convert it to a booltrueregardless of what type the user declared it should be in thefunctionSignature.I would consider this PR to be a HOTFIX, to get something out the door quickly.
Issue / Notion doc (if applicable)
n/a
Testing
npm run testto run the unit test suite. Notice how there are failing tests.There are a handful of failing unit tests in this PR, which should either be commented out, OR fixed, in order to merge.
If we decide to comment out for now, that is an explicit decision to merge with known bugs in this critical code.
The ideal scenario is to refactor the
encodeFunctionfunction, to pair up the individual types with their values, so we can make a more informed decision on when to "convert" from a string"true"/"false"to a booleantrue/false, instead of optimistically always assuming that those values are supposed to bebool, as in this PR's implementation.Screenshots (if applicable)
n/a