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 "empty data" check to AbiCoder::decodeLog #2686

Closed
wants to merge 3 commits into from

Conversation

Anaphase
Copy link

@Anaphase Anaphase commented Apr 15, 2019

Description

This PR address an issue discussed in (and related to) #2582. AbiCoder::decodeLog is not performing the same "empty data" check that PR #2549 added to EventLogDecoder::decode.

I wasn't able to get the tests to run properly on my machine... not sure what I'm doing wrong there. But this is such a minor change I'm not sure it's necessary? I was able to test this locally with Ganache and it works just fine.

Type of change

  • Bug fix

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no warnings.
  • I have updated or added types for all modules I've changed
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run test in the root folder with success and extended the tests if necessary.
  • I ran npm run build in the root folder and tested it in the browser and with node.
  • I ran npm run dtslint in the root folder and tested that all my types are correct
  • I have tested my code on an ethereum test network.

…ters() to throw an error when event data is empty
@Anaphase Anaphase changed the title Fixes bug in web3.eth.abi.decodeLog() method that caused decodeParame… Add "empty data" check to AbiCoder::decodeLog Apr 15, 2019
@Anaphase
Copy link
Author

Well, I still can't get the tests to run locally, but I can see the output on Travis so I'll try to use that and update / add a test case for this. FYI this is what I see when I run npm run test locally:

lerna info Executing command in 18 packages: "npm run test"
lerna ERR! npm run test exited 1 in 'web3-utils'
lerna ERR! npm run test stdout:

> web3-utils@1.0.0-beta.52 test ~/Sites/web3.js/packages/web3-utils
> jest

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |        0 |        0 |        0 |        0 |                   |
----------|----------|----------|----------|----------|-------------------|

lerna ERR! npm run test stderr:
FAIL tests/src/SoliditySha3Test.js
  ● Test suite failed to run

    ~/Sites/web3.js/packages/web3-utils/tests/src/SoliditySha3Test.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import isArray from 'lodash/isArray';
                                                                                                    ^^^^^^^

    SyntaxError: Unexpected identifier

      at ScriptTransformer._transformAndBuildScript (../../node_modules/@jest/transform/build/ScriptTransformer.js:471:17)
      at ScriptTransformer.transform (../../node_modules/@jest/transform/build/ScriptTransformer.js:513:25)

Jest isn't transforming es6 imports properly or something... 🤔

@Anaphase
Copy link
Author

Welp, turns out I don't really know what I'm doing here so maybe someone more familiar with the codebase can take over this PR. I'm not really sure the best way to handle this issue anyway.

@nivida nivida added Enhancement Includes improvements or optimizations In Progress Currently being worked on labels Apr 16, 2019
@nivida
Copy link
Contributor

nivida commented Apr 16, 2019

@Anaphase I will do that for you. But will close this PR for now. The current planning discussion will decide when exactly this will get fixed. #2684

@nivida nivida closed this Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Includes improvements or optimizations In Progress Currently being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants