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

Implement Contract public interfaces #4678

Merged
merged 26 commits into from
Jan 28, 2022
Merged

Conversation

nazarhussain
Copy link
Contributor

Description

Add all public interfaces for the Contract package.

Fixes #4564

Type of change

  • New feature (non-breaking change which adds functionality)

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.

@nazarhussain nazarhussain self-assigned this Jan 10, 2022
@nazarhussain nazarhussain added the 4.x 4.0 related label Jan 10, 2022
>(abi: AbiFunctionFragment, params: unknown[], options?: Options, block?: BlockNumberOrTag) {
return hexToNumber(
await this.requestManager.send({
method: 'eth_estimateGas',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be using web3Eth.estimateGas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no both. I want to avoid the circular dependency.

If web3-eth is using web3-eth-contract package then we can't use the other way around. To provide web3.eth.Contract interface to users that dependency is required.

Suggest if you have any other workaround?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to avoid having multiple implementations of the RPC methods, so we don't repeat the mistakes of 1.x. I guess that means splitting the rpc wrappers into a separate package (web3-eth-rpc-methods)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think web3-eth-tx may solve this issue by having common code that can avoid the circular dependency between web3-eth and web3-eth-contract package.

Copy link
Contributor

Choose a reason for hiding this comment

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

web3-eth-tx doesn't exist anymor. The tx util functions also needed access to Web3Eth methods, so it was made apart of Web3Eth for the same circular dependency issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, I don't see any other way to have that small duplicate code to avoid circular dependency. Maybe as things shape up we may find some other solution. If you think of any other way around, please share.

@jdevcs
Copy link
Contributor

jdevcs commented Jan 11, 2022

Could you check CI Build / unit is also failing

@nazarhussain
Copy link
Contributor Author

@jdevcs Yes as mentioned this PR is WIP, so had not looked into tests yet. Related to your comments about EIP-1559 transactions, I used the existing implementation as reference. I wonder why it's already not supporting EIP-1559, and if not then should we include it in 4.x or not.

@jdevcs
Copy link
Contributor

jdevcs commented Jan 12, 2022

@jdevcs Yes as mentioned this PR is WIP, so had not looked into tests yet. Related to your comments about EIP-1559 transactions, I used the existing implementation as reference. I wonder why it's already not supporting EIP-1559, and if not then should we include it in 4.x or not.

Its imp to support eip1559 in 4.x for contract Txs. If its increasing scope of current PR , will be ok to open new issue.

@nazarhussain
Copy link
Contributor Author

@jdevcs Yes already added it to existing issue #4685

@nazarhussain nazarhussain changed the title [WIP] Implement Contract public interfaces Implement Contract public interfaces Jan 19, 2022
@nazarhussain nazarhussain marked this pull request as ready for review January 19, 2022 09:05
@nazarhussain nazarhussain merged commit e6d4b23 into 4.x Jan 28, 2022
@nazarhussain nazarhussain deleted the nh/4564-contract-methods branch January 28, 2022 08:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4.x 4.0 related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants