Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

Conversation

@marcj
Copy link

@marcj marcj commented Jun 3, 2021

The actual code exports as default a AbiCoder instance. In the previous version the class itself was exported via its name, which breaks runtime code.

Correctly used it looks like that:

import abiCoder from 'web3-eth-abi';

const data = abiCoder.encodeParameters(...);

The new typings reflects that now correctly.

Description

Please include a summary of the changes and be sure to follow our Contribution Guidelines.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

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 new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I ran npm run test:cov and my test cases cover all the lines and branches of the added code.
  • I ran npm run build and tested dist/web3.min.js in a browser.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.

The actual code exports as default a AbiCoder instance. In the previous version the class itself was exported via its name, breaks runtime code.

Correctly used it looks like that:

```typescript
import abiCoder from 'web3-eth-abi';

const data = abiCoder.encodeParameters(...);
```

The new typings reflects that now correctly.
@GregTheGreek
Copy link
Contributor

Thanks for the PR, I haven't tested this specifically yet, but from my understanding this doesn't quite solve it and we have a deeper issue that requires breaking the api.

AFAIIK Typescript doesn't have a way to export a instantiated classes. Specifically, abicoder looks like:

const coder = new AbiCoder();
module.exports = coder;

@marcj
Copy link
Author

marcj commented Jun 3, 2021

For me this solved it. We can not use the class AbiCoder in user land as you indicated since it isn't exported in the javascript file.

AFAIIK Typescript doesn't have a way to export a instantiated classes

My PR provides precisely that.

but from my understanding this doesn't quite solve it and we have a deeper issue that requires breaking the api

I don't see a deeper issue here. The published TS api is broken and unusable, so fixing it is the only way. No need to change the JS.

@GregTheGreek
Copy link
Contributor

For me this solved it. We can not use the class AbiCoder in user land as you indicated since it isn't exported in the javascript file.

AFAIIK Typescript doesn't have a way to export a instantiated classes

My PR provides precisely that.

but from my understanding this doesn't quite solve it and we have a deeper issue that requires breaking the api

I don't see a deeper issue here. The published TS api is broken and unusable, so fixing it is the only way. No need to change the JS.

Let me run this tomorrow, last time someone brought this up, it actually didn't solve the problem. But perhaps we were all mistaken!

@marcj
Copy link
Author

marcj commented Jun 8, 2021

👋 Alright, let me know if you need anything else.

@onchainguy-btc
Copy link

Would be awesome to see this merged, having the same issue 👐

@chancehudson
Copy link

I'd like to see this merged as well.

import AbiCoder from 'web3-eth-abi'
const encodeFunctionSignature = (AbiCoder as any).encodeFunctionSignature.bind(AbiCoder)
const decodeParameters = (AbiCoder as any).decodeParameters.bind(AbiCoder)

Ideally the export would simply be the functions, not a class. These are stateless utilities, why am I having to bind them?

I'll definitely take this PR as a fix though, it will let me remove the any cast.

@github-actions
Copy link

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

@github-actions github-actions bot added the Stale Has not received enough activity label Aug 29, 2021
@sambacha
Copy link

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

lgtm

@github-actions github-actions bot removed the Stale Has not received enough activity label Sep 1, 2021
@marcj
Copy link
Author

marcj commented Sep 18, 2021

can you please merge that?

@github-actions
Copy link

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

@github-actions github-actions bot added Stale Has not received enough activity and removed Stale Has not received enough activity labels Nov 18, 2021
@github-actions
Copy link

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

@github-actions github-actions bot added the Stale Has not received enough activity label Jan 18, 2022
@marcj
Copy link
Author

marcj commented Jan 18, 2022

Can you please merge that?

@github-actions github-actions bot removed the Stale Has not received enough activity label Jan 19, 2022
@github-actions
Copy link

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

@github-actions github-actions bot added the Stale Has not received enough activity label Mar 20, 2022
@mconnelly8 mconnelly8 removed the Stale Has not received enough activity label Mar 21, 2022
@jdevcs jdevcs added 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels Apr 7, 2022
): { [key: string]: string };
}

declare var coder: AbiCoder;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
declare var coder: AbiCoder;
declare const coder: AbiCoder;

I think you need to use const here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why need? If it doesn't match you coding style, you can change it afterwards, or close and commit yourself. For me its only important that this bug gets fixed, not how.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just fix it, please. and also check tests. They do not pass after your changes.

Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from feedback from @avkos LGTM

@marcj marcj closed this Apr 11, 2022
@chancehudson chancehudson mentioned this pull request Apr 12, 2022
14 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

1.x 1.0 related issues Review Needed Maintainer(s) need to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants