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 "Miner and DAG" management API's implemented (web3-eth-miner) #2660

Merged
merged 29 commits into from
Apr 16, 2019

Conversation

princesinha19
Copy link
Contributor

Description

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

Fixes #2637 Partially

Type of change

  • New feature

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 7, 2019

Coverage Status

Coverage increased (+0.05%) to 95.545% when pulling 4ab586e on princesinha19:miner-methods into bb891b2 on ethereum:1.0.

@princesinha19 princesinha19 changed the title Geth miner management API's implemented (web3-eth-miner) Geth "Miner and DAG" management API's implemented (web3-eth-miner) Apr 8, 2019
@nivida nivida added Feature Request In Progress Currently being worked on labels Apr 8, 2019
@princesinha19 princesinha19 mentioned this pull request Apr 8, 2019
12 tasks
@joshstevens19
Copy link
Contributor

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.

https://github.com/ethereum/web3.js/blob/1.0/packages/web3-eth/types/index.d.ts

Please add it

@princesinha19
Copy link
Contributor Author

princesinha19 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.

https://github.com/ethereum/web3.js/blob/1.0/packages/web3-eth/types/index.d.ts

Please add it

Why it is so, web3-eth-miner is an independent module and it has it's own typing class. So, what's the need to check type in web3-eth. This is same for web3-eth-personal and all other modules. I think this dependency should be removed. Correct me if I am wrong @joshstevens19.
@nivida

@joshstevens19
Copy link
Contributor

joshstevens19 commented Apr 12, 2019

@princesinha19 @nivida that's not true. If you want to get to web3.eth.personal through the main web3 class - https://web3js.readthedocs.io/en/1.0/web3-eth.html#personal

In code you have to write web3.eth.personal if personal is not on the web3.eth typing's you can never get to it https://github.com/ethereum/web3.js/blob/1.0/packages/web3-eth/types/index.d.ts#L50

I am not saying check types in web3.eth just exposing them as the above link shows ^^ BUT 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 that's the case then great and shouldn't add them to web3.eth.

If you remove this it will break and not compile in typescript. You need to expose this on the web3-eth typing's interface if you want people to be able to get to it through there like personal. If you don't want web3-eth-miner to ever be able to be accessed through web3.eth and import only then that's fine.

We can not remove web3.eth.personal as it will break what the docs say which people support and then break TS apps.

@princesinha19
Copy link
Contributor Author

@joshstevens19 can you please review this too?

@joshstevens19
Copy link
Contributor

You have some conflicts to resolve @princesinha19 👍

@nivida nivida merged commit d3bc225 into web3:1.0 Apr 16, 2019
@princesinha19 princesinha19 mentioned this pull request Apr 17, 2019
12 tasks
@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
Labels
Feature Request In Progress Currently being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add all management API methods of Geth
4 participants