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

Custom TransactionSigner and eth-account module improvements. #2367

Merged
merged 84 commits into from
Feb 27, 2019
Merged

Conversation

nivida
Copy link
Contributor

@nivida nivida commented Feb 10, 2019

Description

This PR fixes #2356 #2378 #1998 #1255 #2300 and will also improve the QA of the module.

Web3 Accounts Refactoring

The eth-accounts module is one of the last modules which didn't got improved during the last refactoring of the core modules.
My idea now is to clean up the eth-accounts module and to add a transactionSigner options property for the eth module.

This sounds like a big refactoring but I will mostly just move the things.

TransactionSigner

It will be possible to create your own class of type TransactionSigner and inject it over the options property. The class name TransactionSigner is a reserved class name and will be used for the internal signer class.

interface TransactionSigner {
  sign(txObject: Transaction): Promise<SignedTransaction>
}

interface SignedTransaction {
  messageHash: string,
  v: string,
  r: string,
  s: string,
  rawTransaction: string
}
Example
const eth = new Eth(provider, {transactionSigner: new HDWalletSigner(...)});

Accounts

The interface of the Accounts class (web3.eth.accounts) will still be the same.

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 the live network.

@nivida nivida added Bug Addressing a bug In Progress Currently being worked on labels Feb 10, 2019
Samuel Furter and others added 23 commits February 18, 2019 17:40
…ted in Accounts module and TransactionSigner simplified
…nd dependency handling updated in the core-method module.
…d just works if you're connected to a Parity node
@nivida nivida removed the In Progress Currently being worked on label Feb 26, 2019
@nivida nivida added the In Progress Currently being worked on label Feb 26, 2019
@coveralls
Copy link

coveralls commented Feb 26, 2019

Coverage Status

Coverage increased (+1.4%) to 94.619% when pulling 13eb73d on issue/2356 into 7f5a390 on 1.0.

this.methodFactory = this.contractModuleFactory.createMethodFactory();
this.abiModel = this.abiMapper.map(abi);
this.transactionSigner = options.transactionSigner;
this.options = options;

Choose a reason for hiding this comment

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

duplicates with line 62

Copy link
Contributor Author

@nivida nivida Feb 27, 2019

Choose a reason for hiding this comment

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

This isn't a duplication. I'm just passing the TransactionSigner dependency over the options. Check out the Eth module for seeing the real reason why I'm doing this. The complete dependency handling will get improved later (removing of the Factory objects etc.).

Copy link

Choose a reason for hiding this comment

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

I mean this assignment this.options = options; it happens twice probably a left over from git merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, I got it wrong. Thanks.

@nivida nivida added In Progress Currently being worked on and removed In Progress Currently being worked on labels Feb 27, 2019
@nivida nivida removed the In Progress Currently being worked on label Feb 27, 2019
@nivida nivida merged commit dc03898 into 1.0 Feb 27, 2019
@nivida nivida deleted the issue/2356 branch February 28, 2019 09:07
@rstormsf
Copy link

rstormsf commented Mar 3, 2019

@nivida Any great example on how to use it?

this.confirmationSubscription = this.subscriptionsFactory
.createNewHeadsSubscription(moduleInstance)
.subscribe(() => {
console.log('newHEAD');

Choose a reason for hiding this comment

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

maybe forgot to remove?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Addressing a bug Enhancement Includes improvements or optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call to account.signTransaction({...}) throw TypeError
6 participants