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

AbiInput and AbiOutput interface update #2478

Merged
merged 5 commits into from
Mar 18, 2019
Merged

AbiInput and AbiOutput interface update #2478

merged 5 commits into from
Mar 18, 2019

Conversation

teef
Copy link

@teef teef commented Mar 8, 2019

AbiInput and AbiOutput interface update

Description

AbiInput and AbiOutput interfaces supplemented with optional components property.
AbiOutput typo fixed.

Type of change

  • Bug fix
  • Enhancement

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.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.529% when pulling 4d6f3ad on Meelabs:1.0 into 970d837 on ethereum:1.0.

@coveralls
Copy link

coveralls commented Mar 8, 2019

Coverage Status

Coverage remained the same at 98.505% when pulling d29ab09 on Meelabs:1.0 into 441135e on ethereum:1.0.

Copy link
Contributor

@joshstevens19 joshstevens19 left a comment

Choose a reason for hiding this comment

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

Good spot on the bad spelling my mistake ha. Thanks for this PR looks good to me.

@joshstevens19 joshstevens19 added the Types Incorrect or missing types label Mar 8, 2019
Copy link
Contributor

@joshstevens19 joshstevens19 left a comment

Choose a reason for hiding this comment

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

oops sorry no it is not - can you fix the tests for the types please. @teef

@teef
Copy link
Author

teef commented Mar 10, 2019

Hi @joshstevens19 . I have fixed typings tests and also extended AbiItem in json-interface-method-to-string-test.ts file to match current definition.
Static property from Web3 class was removed. To implement utils as a static property it is required to modify Web3 class in Web3.js file. I will provide my proposal in next PR.

@nivida
Copy link
Contributor

nivida commented Mar 11, 2019

@teef The static property got removed because there is also a non-static property utils and I'm recommending to use the web3-utils module directly if you just want to use the utility methods.

@teef
Copy link
Author

teef commented Mar 13, 2019

Hi @nivida.
Thanks for explanation regarding utils.

At the moment I see that utilities originate from web3-utils. This module aggregates all of them and expose as named exports. Furthermore utilities are imported in node_modules\web3\src\Web3.js file and used in Web3 class constructor. This means that each and every Web3 instance will have entire utils package attached. Apart of a fact that this may not be super efficient in terms of resource utilization, I thought that having this accessible as static property is closer to OOP paradigm.

Regardless of the decision I saw some inconsistency between Utils TS definition, documentation and implementation. There are some methods missing in current utils module. Please find more details below. Description originates from this PR: Meelabs#1

Changes: Meelabs@ae012bb

Missing utilities added to utils property of Web3 class:

isBigNumber(): added
leftPad(): added for compatibility with TS Interface
rightPad(): added for compatibility with TS Interface
unitMap(): added

Known issues:

testAddress(): defined but implementation is missing
testTopic(): defined but implementation is missing
both methods do not exist in documentation of version 1.0.

I would grateful if you can put some comment on that.

Additionally let me know if the PR can be merged?

@nivida
Copy link
Contributor

nivida commented Mar 18, 2019

@teef Thanks for your feedback!

Yes, that's true the utils were available as static property over the Web3 class. I'm not a big fan of static properties because it's always hard to figure out in a bigger architecture where exactly the static methods and properties are used. The only reason I will use them is for factory methods of a VO (e.g.: Transaction.from(...))
With the removing of the static property utils do I enforce the developers to use the utils module directly which will be better for the bundle size. The long-term goal is to create the Web3 Namespace and the Web3 Types Module which will remove the umbrella class and replace the utils module with clear VO's.

Feel free to give your feedback in the gist files!

@nivida nivida merged commit f66e080 into web3:1.0 Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Types Incorrect or missing types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants