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

fix: send tx for injected providers #5592

Closed

Conversation

mpetrunic
Copy link
Contributor

@mpetrunic mpetrunic commented Nov 4, 2022

Description

Please include a summary of the changes and be sure to follow our Contribution Guidelines.

Implementation wrongly assumes every provider has supportsSubscriptions method which is not true for injected providers.

No idea how to write test for that.

Fixes #5591

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist for 1.x:

  • 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:cov and my test cases cover all the lines and branches of the added code.
  • I ran npm run build with success.
  • I have tested the built 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.

Checklist for 4.x:

  • I have selected the correct 4.x base branch.
  • Within the description, the feature or issue is discussed in detail or an issue is linked.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added any required tests for the changes I made
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged and published in downstream modules.
  • I ran yarn successfully
  • I ran yarn lint successfully
  • I ran yarn build:web successfully
  • I ran yarn test:unit successfully
  • I ran yarn test:integration successfully
  • I ran compile:contracts successfully
  • I have tested my code.
  • I have updated the corresponding CHANGELOG.md file in the packages I have edited.

@mpetrunic mpetrunic added the 4.x 4.0 related label Nov 4, 2022
@mpetrunic mpetrunic requested a review from a team November 4, 2022 13:36
@Muhammad-Altabba
Copy link
Contributor

Muhammad-Altabba commented Nov 8, 2022

Thanks for your contribution.
And I think this could be tested by initializing a provider and then assigning undefined to its supportsSubscriptions method. And then if sending a transaction did not throw, this means that the code works well.
Or what do you think?

@mpetrunic
Copy link
Contributor Author

mpetrunic commented Nov 9, 2022

Thanks for your contribution. And I think this could be tested by initializing a provider and then assigning undefined to its supportsSubscriptions method. And then if sending a transaction did not throw, this means that the code works well. Or what do you think?

I'm having trouble running integration tests in web3-eth package and I cannot figure out if they are being executed in CI

Tried this, but tests are timing out :(

const provider = new HttpProvider(getSystemTestProvider())
const injectedProvider: EIP1193Provider<EthExecutionAPI> =  {
	request: provider.request.bind(provider),
}
web3Eth = new Web3Eth(injectedProvider);

@Muhammad-Altabba
Copy link
Contributor

Thanks for your contribution. And I think this could be tested by initializing a provider and then assigning undefined to its supportsSubscriptions method. And then if sending a transaction did not throw, this means that the code works well. Or what do you think?

I'm having trouble running integration tests in web3-eth package and I cannot figure out if they are being executed in CI

Tried this, but tests are timing out :(

const provider = new HttpProvider(getSystemTestProvider())
const injectedProvider: EIP1193Provider<EthExecutionAPI> =  {
	request: provider.request.bind(provider),
}
web3Eth = new Web3Eth(injectedProvider);

This is interesting. It could be another issue that we need to take care of 😄 .
I would suggest pushing the full code that contains the test case, and then I would probably debug it locally.
Or you may open another issue to investigate this matter. And probably merge this MR if you could test it locally at another project, for example.
Or what do you suggest?

@mpetrunic
Copy link
Contributor Author

Thanks for your contribution. And I think this could be tested by initializing a provider and then assigning undefined to its supportsSubscriptions method. And then if sending a transaction did not throw, this means that the code works well. Or what do you think?

I'm having trouble running integration tests in web3-eth package and I cannot figure out if they are being executed in CI
Tried this, but tests are timing out :(

const provider = new HttpProvider(getSystemTestProvider())
const injectedProvider: EIP1193Provider<EthExecutionAPI> =  {
	request: provider.request.bind(provider),
}
web3Eth = new Web3Eth(injectedProvider);

This is interesting. It could be another issue that we need to take care of smile . I would suggest pushing the full code that contains the test case, and then I would probably debug it locally. Or you may open another issue to investigate this matter. And probably merge this MR if you could test it locally at another project, for example. Or what do you suggest?

I tested locally, and it did send transaction. Will open another PR with just test case.

@Muhammad-Altabba
Copy link
Contributor

Dear @mpetrunic
I think this MR is all great and just needs to be merged. Or does it pending for something?
Thanks,

@mpetrunic mpetrunic force-pushed the fix/send-tx-for-injected-providers branch from b56c9d8 to 6f0f7cb Compare November 21, 2022 08:32
@mpetrunic
Copy link
Contributor Author

Dear @mpetrunic I think this MR is all great and just needs to be merged. Or does it pending for something? Thanks,

Seems like I stopped signing commits on my new laptop. Fixed and replaced with signed commit.

Also wasn't sure if we are waiting on #5309

@Muhammad-Altabba
Copy link
Contributor

Dear @mpetrunic I think this MR is all great and just needs to be merged. Or does it pending for something? Thanks,

Seems like I stopped signing commits on my new laptop. Fixed and replaced with signed commit.

Also wasn't sure if we are waiting on #5309

Here is an MR for #5309. It contains tests for both ganache and in3. I also merged your commit from this MR into my MR.

(By the way, I could not cherry-pick this commit because it is on a private branch. So, I made a small MR from your private branch to my branch and then merged it)

@mpetrunic
Copy link
Contributor Author

Superseded by #5652

Here is an MR for #5309. It contains tests for both ganache and in3. I also merged your commit from this MR into my MR.

(By the way, I could not cherry-pick this commit because it is on a private branch. So, I made a small MR from your private branch to my branch and then merged it)

No worries, I don't care about that 😛
Just for info in the future, to do that, you have to add the contributors fork as remote and fetch his branch^^

@mpetrunic mpetrunic closed this Nov 24, 2022
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