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

Use only web3 as dependency #1

Closed
mpetrunic opened this issue Oct 4, 2022 · 2 comments · Fixed by #27
Closed

Use only web3 as dependency #1

mpetrunic opened this issue Oct 4, 2022 · 2 comments · Fixed by #27
Assignees

Comments

@mpetrunic
Copy link
Member

Importing types and functionality from a bunch of different web3 packages are making it hard to link web3 and test changes against the dev version of web3.

so instead of:

import Contract from "web3-contract";
import Web3 from "web3"

we should only have:

import web3 from "web3";

const contract = new web3.eth.Contract()

if we can't do that, we should raise an issue on the web3 repo to export missing functionality.

@spacesailor24
Copy link
Contributor

spacesailor24 commented Oct 5, 2022

So there's actually a misconception regarding the plugin's use of web3 to re-declare the module:

declare module 'web3' {
	interface Web3 {
		chainlink: ChainlinkPlugin;
	}
}

I don't think the above code is the responsibility of the plugin, it actually belongs to the user using the ChainlinkPlugin. The registerPlugin method exists on the Web3Context class and not Web3, so there's no guarantee ChainlinkPlugin is going to be added to a Web3 instance

To explain this better, consider the following:

import { Web3Context } from 'web3-core/dist/web3_context';

import { ChainlinkPlugin } from '../../src/index';

declare module 'web3-core' {
	interface Web3Context {
		chainlink: ChainlinkPlugin;
	}
}

describe('Chainlink Plugin Tests', () => {
	it('should register ChainlinkPlugin and make the getPrice call', async () => {
		const web3Context = new Web3Context('https://rpc.ankr.com/eth_rinkeby');
		web3Context.registerPlugin(new ChainlinkPlugin(AggregatorV3InterfaceABI, aggregatorAddress));
		const price = await web3Context.chainlink.getPrice();
		expect(Object.keys(price)).toEqual(
			expect.arrayContaining([
				'roundId',
				'answer',
				'startedAt',
				'updatedAt',
				'answeredInRound',
			]),
		);
	});
});

In this example there is no dependency on Web3 package, only web3-core and web3-eth-contract, but the larger point being made here is that registerPlugin can be called on any class that extends Web3Context (or in this case Web3Context itself), so the plugin author cannot handle the module re-declaration. Instead, the user must re-declare the module for whatever class they are adding the plugin to


Aside from the above, I don't think we should rely on the web3 package to provide us with the classes and types needed for the plugin because the web3 package pulls in the entire Web3.js library, and this would greatly increase the build size of the plugin as tree shaking is not possible with the Web3 class

@mpetrunic
Copy link
Member Author

mpetrunic commented Oct 6, 2022

Module augmentation

Without plugin author augmenting types, you have two options:

  • either user will augment themselves -> trust me, they won't, they will just start using ts-ignore or casting to any
  • or you have to force the user to use a new augmented instance returned from registerPlugin -> could be a viable path forward but it will require users to pass an instance of web3 around instead of creating a new one 🤷🏻‍♂️

I would suggest that we continue this discussion on some issue or discussion board on web3.js

Original issue

This issue is blocked by web3/web3.js#5508 where I moved discussion,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants