Fix typing compatibility when linking a contract to a context

[Muhammad-Altabba]

Description

A suggested edit addressing the discussion points mentioned at #5393 (comment)

This is an MR to another MR (to #5393)

The basic idea is to make the first generic parameter of Web3Context default to be any and to use this in all relevant places to allow for flexibility when accepting the type Web3Context. As at Contract the method link<T extends Web3Context<API, RegisteredSubs>> it becomes link<T extends Web3Context> which is basically equivalent to link<T extends Web3Context<any,any>> which allow for accepting any Web3Context that could be defined by any plugin. However, to achieve that, some other types of changes were required and they are done in this MR. But those changes are best to look at as a suggestion that encourages discussions...

So, this MR is a draft suggestion that could be taken as-is. Or its commit(s) could be cherry-pick from, and then modified as needed.

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

[spacesailor24]

API should be passed here otherwise the specification of ChainlinkPluginAPI is meaningless. The purpose of specifying the ChainlinkPluginAPI (as we do with EthExecutionApi) is to provide type safety when using the requestManager e.g. API specifies what methods are available to call and their corresponding typed params when doing things like so:

which gives us type safety like so:

Removing <API, RegisteredSubs> for link means API gets default to unknown as you've added API extends Web3APISpec = unknown, which yields:

which means TypeScript is no longer aware of what's available for the requestManager to call, what params the specified method takes, and what the return type of the request is

[Muhammad-Altabba]

Actually, requestManager and link<T extends Web3Context> are separate. And so not passing API to link<T extends Web3Context> does not affect requestManager. And I kept it untouched as-is:

export class Web3RequestManager<
	API extends Web3APISpec = EthExecutionAPI,
> extends Web3EventEmitter<{
	[key in Web3RequestManagerEvent]: SupportedProviders<API> | undefined;
}> {
...

And so the safe typing is working well:

And, if I provided no params to eth_getBalance then running yarn build would give the following error:

web3-eth: src/rpc_methods.ts(99,3): error TS2322: Type '[]' is not assignable to type '[address: string, blockNumber: BlockNumberOrTag]'.

So, I think you just had a cashing problem when checking out this branch. Kindly try again by running yarn build. And close vscode and reopen it, if it was still not showing you the correct error...

And let me know, please, if there is any type-safe feature that is not working as expected.

[spacesailor24]

The third screenshot is the inferred type when using the requestManager for the plugin. The EthExecutionApi (i.e. the requestManager when used within the web3-eth package) still resolves as expected with your approach, but the same API type resolution does not occur for ChainlinkPluginAPI - this is what #5480 is attempting to solve. I don't think making use of unknown is the best solution for this issue as we do know what the API is because we specify it. The changes in #5480 should allow us to pass an API that's not EthExecutionApi (or extends it as Web3EthExecutionApi does), but as mentioned on our call and below, our code around subscriptions/logs is hardcoded to handle EthExecutionApi and therefore throws type errors when trying to use an API that doesn't have eth_subscribe

[Muhammad-Altabba]

If the plugin writer would like to use this._requestManager, then the plugin api would need to be something like:

// This is defined as a type, because defining this as an interface will fool the compiler to not well-detect
//  the API at EthExecutionAPI when passing it to Web3PluginBase.
export type ChainlinkPluginAPI = EthExecutionAPI & {
	getPrice: () => Promise<Price>;
};

And then all the type-safety for the request manager would work inside the plugin.

---

However, I also I caught the source of confusion for getPrice. This has nothing to do with the ChainlinkPluginAPI. This should be only inside ChainlinkPlugin. And, actually, when the user writes web3.chainlink, it should be of the type ChainlinkPlugin and not ChainlinkPluginAPI.
So, 2 places needed to be fixed for that:
First is the definition of the chainlink variable:

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

And is that the type of ChainlinkPluginAPI should contain only the JSON RPC methods for the connected node. So, ti should not contain getPrice. But if the node provides some non-standard JSON RPC method. Or JSON RPC methods for substrate, for example (if the plugin was expanded later for such a possibility).

---

So, I pushed a commit that addresses those points in addition to a few enhancements....
Please, let me know if you have any other concerns about the provided solution. And if so, please provide a code snippet of what you would like to do exactly and I will help investigating that.

[spacesailor24]

Yea I was totally thinking about ChainlinkPluginAPI incorrectly - it's not needed given that there are no JSON RPC methods being added as you've mentioned. I think it's fine for Web3PluginBase to just default to Web3APISpec (so the user doesn't have to worry about passing the generic if they're not adding a JSON RPC API) as added by this commit

The rest of the changes you've made in this PR (namely around Web3Context) I don't think are needed for #5393, but probably do need to be incorporated into #5480. #5480 is actually trying to solve an issue not strictly related to #5393 and that's fixing the existing architecture around Web3Context and the RequestManager when you pass an API that doesn't extend EthExecutionApi - which I don't believe should be a requirement when a plugin/user wants to add JSON RPC methods (it's currently a requirement since code around log subscriptions is currently hardcoded to use EthExecutionApi as mentioned here) I've also been thinking about this incorrectly, the issue I thought we had regarding instantiating a Web3Context with an API that does not extend EthExecutionApi is non-existent as demonstrated by the following:

export type FooApi = {
	foo_bar: (arg1: string, arg2: number) => boolean;
	bar_foo: () => string;
}

const web3Context = new Web3Context<FooApi>('http://127.0.0.1:8545');
web3Context.requestManager.send({
	method: 'foo_bar',
	params: [] // This errors as expected (see screenshot)
})

web3Context.requestManager.send({
	method: 'bar_foo',
	params: []
})

The error I was actually seeing is fixed by extending EthExecutionApi like you've mentioned above:

export type ChainlinkPluginAPI = EthExecutionAPI & {
	getPrice: () => Promise<Price>;
};

Even though this ChainlinkPluginAPI is not actually needed for the plugin (since we're not adding support for any new JSON RPC methods), for the sake of this example, let's just assume it's needed. When attempting to do

interface Price {
	roundId: string;
	answer: string;
	startedAt: string;
	updatedAt: string;
	answeredInRound: string;
}

interface ChainlinkPluginAPI extends Web3APISpec {
	getPrice: () => Promise<Price>;
}

declare module 'web3/dist/web3' {
	interface Web3 {
		chainlink: ChainlinkPluginAPI;
	}
}

export class ChainlinkPlugin extends Web3PluginBase<Web3APISpec> {
	public pluginNamespace = 'chainlink';

	protected readonly _contract: Contract<typeof AggregatorV3InterfaceABI>;

	public constructor(abi: ContractAbi, address: Address) {
		super();
		this._contract = new Contract(abi, address);
	}

	public async getPrice() {
		if (this._contract.currentProvider === undefined) this._contract.link(this);
		return this._contract.methods.latestRoundData().call();
	}
}

You receive the following error for the line

if (this._contract.currentProvider === undefined) this._contract.link(this);

Argument of type 'this' is not assignable to parameter of type 'Web3Context<EthExecutionAPI, { logs: typeof LogsSubscription; newHeads: typeof NewHeadsSubscription; newBlockHeaders: typeof NewHeadsSubscription; }>'.
  Type 'ChainlinkPlugin' is not assignable to type 'Web3Context<EthExecutionAPI, { logs: typeof LogsSubscription; newHeads: typeof NewHeadsSubscription; newBlockHeaders: typeof NewHeadsSubscription; }>'.
    Types of property '_requestManager' are incompatible.
      Type 'Web3RequestManager<ChainlinkPluginAPI>' is not assignable to type 'Web3RequestManager<EthExecutionAPI>'.
        Type 'ChainlinkPluginAPI' is missing the following properties from type 'EthExecutionAPI': eth_getBlockByHash, eth_getBlockByNumber, eth_getBlockTransactionCountByHash, eth_getBlockTransactionCountByNumber, and 44 more.ts(2345)

I was thinking ChainlinkPluginAPI shouldn't need to extend EthExeuctionAPI in this case because it would add a bunch of JSON RPC methods that aren't going to be used for the plugin. However, ChainlinkPluginAPI needs to extend EthExeuctionAPI because it's using web3-eth-contract which depends on JSON RPC methods defined by EthExeuctionAPI to perform certain actions. The error in my thinking was that you don't need EthExecutionAPI just to call methods on a deployed contract (as the example Chainlink plugin is doing), but that would entail removing functionality from the web3-eth-contract to get it to work which just doesn't make sense

[Muhammad-Altabba]

Great @spacesailor24 👍
I am happy to see that this MR helped resolve the 2 blockers for #5393 and provided clarifications 😄

And so it is time to merge this MR.

However, because I think that, there are some copied ideas from this MR to #5393 (and this is also the reason for having conflicts in the MR with the destination branch). I suggest you do one of the following:

• Ideally, it would be better to preserve the history and the orders of the commits. And so I suggest you to rebase your branch to an earlier commit and then re-push the commits from this branch and from your branch in the order they came. And before doing this I recommend that you save the commits from both branches and the date of each one. And also to have a full copy of the local folder, that you can use in case things were messed-up. This will keep the contribution clean of who did what at which time and it would be easier to trace if later needed and this is the best practice, I think.
• The other option is easier but will vanish the valuable history. It is to resolve the conflicts in this MR and merge it with yours. This is not the best option because there were some ideas copied from this MR to the other MR in parallel instead of merging it early. But I am OK with that if you faced issues when trying the first option.

And by the way, I also pushed some more enhancements and comments to the code so when someone later would implement a plugin would have a clearer understanding, I hope. And this included 2 classes Web3PluginBase and Web3EthPluginBase 😄.