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

Geth Admin management API's implemented (web3-eth-admin) #2657

Merged
merged 35 commits into from
Apr 15, 2019

Conversation

princesinha19
Copy link
Contributor

@princesinha19 princesinha19 commented Apr 6, 2019

Description

I implemented all admin management API's provided by geth.
https://github.com/ethereum/go-ethereum/wiki/Management-APIs#list-of-management-apis

In this PR some of the the code for type is missing. So, I will update it further.

Fixes #2637 Partially

Type of change

  • New feature
  • 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

coveralls commented Apr 6, 2019

Coverage Status

Coverage increased (+0.06%) to 95.439% when pulling 5c4633a on princesinha19:admin_methods into 441c309 on ethereum:1.0.

Copy link
Contributor

@nivida nivida left a comment

Choose a reason for hiding this comment

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

Could you extend the documentation for this module? :)

packages/web3-eth-admin/src/Admin.js Outdated Show resolved Hide resolved
@nivida nivida added Feature Request In Progress Currently being worked on labels Apr 6, 2019
@princesinha19
Copy link
Contributor Author

princesinha19 commented Apr 6, 2019

Yes sure @nivida. I will start documenting it.
Should I add document in this PR or separate PR?

@nivida
Copy link
Contributor

nivida commented Apr 6, 2019

I would add the docs directly here to this PR. Thanks! :)

@nivida
Copy link
Contributor

nivida commented Apr 8, 2019

@princesinha19 Sorry if I annoy you but could you also update the other method object names in the same pattern as the already changed methods?:)

@princesinha19
Copy link
Contributor Author

@nivide no problem. I will change it. Can you point me to the methods?
I changed setSolcMethod, StartRPCMethod, StopRPCMethod, StartWSMethod, StopWSMethod, AddPeerMethod, GetAdminPeerMethod, AdminNodeInfoMethod, AdminDataDirectoryMethod.

should I change GetAdminPeerMethod to GetPeerMethod?

@nivida
Copy link
Contributor

nivida commented Apr 9, 2019

If this has to get to through web3.eth you have to add this to the Eth typings class or it wont be exposed in the global scope when ts users try to use it.

The eth management modules should not get added to the web3.eth module this would blow up the bundle size for "normal" DApp's. :)

@princesinha19
Copy link
Contributor Author

princesinha19 commented Apr 9, 2019

The eth management modules should not get added to the web3.eth module this would blow up the bundle size for "normal" DApp's. :)

Right.

@nivida I can see. all the methods of web3-eth-personal, web3-eth-accounts are added in Eth typings class of web3-eth.

@princesinha19
Copy link
Contributor Author

princesinha19 commented Apr 10, 2019

I overloaded the types and also did all the requested changes.

After overloading the method some error got occurred in build pipeline. Could please take a look at build pipeline @joshstevens19.

@joshstevens19
Copy link
Contributor

@nivida @princesinha19 If this is a standalone module then great but in regards to what you said about web3-eth-personal, web3-eth-accounts if you read my comment here it will make sense.

#2660 (comment)

I think all your PRs are standalone modules the user should only be able to get to it if they import the module and not through the public interface. If thats the case then great. We can't and should not remove the web3-eth-personal, web3-eth-accounts dependency as in the public API docs it states you can get to it through web.eth interface. Which i know a lot of people do.

https://web3js.readthedocs.io/en/1.0/web3-eth.html#personal
https://web3js.readthedocs.io/en/1.0/web3-eth.html#accounts

👍

@princesinha19
Copy link
Contributor Author

I am okay with adding it to web3.eth too. But, I agree with @nivida that it will unnecessarily increase the bundle size. So, I think we can separate the management API's and will keep it standalone modules.
what's your thinking @nivida & @joshstevens19?

@joshstevens19
Copy link
Contributor

joshstevens19 commented Apr 12, 2019

Yeah i agree with that, keeping it separate! @princesinha19 👍 Also i forgot to say brilliant work on the PRs done 👍

packages/web3-eth-admin/rollup.config.js Outdated Show resolved Hide resolved
packages/web3-eth-admin/src/Admin.js Outdated Show resolved Hide resolved
@nivida nivida merged commit 5e4724b into web3:1.0 Apr 15, 2019
@princesinha19 princesinha19 deleted the admin_methods branch April 16, 2019 08:50
@nivida nivida mentioned this pull request Apr 29, 2019
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add all management API methods of Geth
4 participants