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

Fix typing compatibility when linking a contract to a context #5416

Merged
spacesailor24 merged 10 commits into from
Oct 5, 2022
Merged

Fix typing compatibility when linking a contract to a context #5416

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9cae1f9
fix typing compatibility when linking a contract to a context
Muhammad-Altabba Sep 6, 2022
3e3cb8e
replace any with unknown at a suggested code for Web3Context
Muhammad-Altabba Sep 8, 2022
d43ac3a
Web3 net system tests (#5457)
nikoulai Sep 27, 2022
6408b57
more fixes and sample calls for the plugin draft
Muhammad-Altabba Sep 28, 2022
d93c86c
tiny code revert for unused variable
Muhammad-Altabba Sep 28, 2022
37cec51
add comments and replace some `any`s with `unknown`s for plugin relat…
Muhammad-Altabba Sep 30, 2022
903d642
5427 contracts unit tests (#5465)
jdevcs Sep 30, 2022
b35c32f
Merge with 4.x
spacesailor24 Oct 5, 2022
4942447
Merge with 5091
spacesailor24 Oct 5, 2022
648a5c7
Merge branch 'wyatt/4.x/5091-chainlink-plugin' into fix/5091/change-w…
spacesailor24 Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions packages/web3-core/src/web3_context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,21 +72,21 @@ export type Web3ContextInitOptions<

export type Web3ContextConstructor<
// eslint-disable-next-line no-use-before-define, @typescript-eslint/no-explicit-any
T extends Web3Context<any>,
T extends Web3Context,
T2 extends unknown[],
> = new (...args: [...extras: T2, context: Web3ContextObject]) => T;

// To avoid circular dependencies, we need to export type from here.
export type Web3ContextFactory<
// eslint-disable-next-line no-use-before-define, @typescript-eslint/no-explicit-any
T extends Web3Context<any>,
T extends Web3Context,
T2 extends unknown[],
> = Web3ContextConstructor<T, T2> & {
fromContextObject(this: Web3ContextConstructor<T, T2>, contextObject: Web3ContextObject): T;
};

export class Web3Context<
API extends Web3APISpec,
API extends Web3APISpec = any,
RegisteredSubs extends {
[key: string]: Web3SubscriptionConstructor<API>;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand All @@ -95,7 +95,7 @@ export class Web3Context<
public static readonly providers = Web3RequestManager.providers;
public static givenProvider?: SupportedProviders<never>;
public readonly providers = Web3RequestManager.providers;
protected _requestManager: Web3RequestManager<API>;
protected _requestManager: Web3RequestManager<API | any>;
protected _subscriptionManager?: Web3SubscriptionManager<API, RegisteredSubs>;
protected _accountProvider?: Web3AccountProvider<Web3BaseWalletAccount>;
protected _wallet?: Web3BaseWallet<Web3BaseWalletAccount>;
Expand Down Expand Up @@ -174,14 +174,14 @@ export class Web3Context<
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
public static fromContextObject<T extends Web3Context<any>, T3 extends unknown[]>(
public static fromContextObject<T extends Web3Context, T3 extends unknown[]>(
this: Web3ContextConstructor<T, T3>,
...args: [Web3ContextObject, ...T3]
) {
return new this(...(args.reverse() as [...T3, Web3ContextObject]));
}

public getContextObject(): Web3ContextObject<API, RegisteredSubs> {
public getContextObject(): Web3ContextObject<API | any, RegisteredSubs> {
return {
config: this.getConfig(),
provider: this.provider,
Expand All @@ -201,7 +201,7 @@ export class Web3Context<
* and then use it to create new objects of any type extended by `Web3Context`.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public use<T extends Web3Context<any>, T2 extends unknown[]>(
public use<T extends Web3Context, T2 extends unknown[]>(
ContextRef: Web3ContextConstructor<T, T2>,
...args: [...T2]
) {
Expand All @@ -220,7 +220,7 @@ export class Web3Context<
/**
* Link current context to another context.
*/
public link<T extends Web3Context<API, RegisteredSubs>>(parentContext: T) {
public link<T extends Web3Context>(parentContext: T) {
Comment on lines -223 to +220
Copy link
Contributor

@spacesailor24 spacesailor24 Sep 27, 2022

Choose a reason for hiding this comment

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

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:

image

which gives us type safety like so:

image

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

image

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

Copy link
Contributor Author

@Muhammad-Altabba Muhammad-Altabba Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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:
image

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.

Copy link
Contributor

@spacesailor24 spacesailor24 Sep 28, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Muhammad-Altabba Muhammad-Altabba Sep 28, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@spacesailor24 spacesailor24 Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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: []
})

image

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

Copy link
Contributor Author

@Muhammad-Altabba Muhammad-Altabba Sep 30, 2022

Choose a reason for hiding this comment

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

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

this.setConfig(parentContext.getConfig());
this._requestManager = parentContext.requestManager;
this.provider = parentContext.provider;
Expand Down
6 changes: 3 additions & 3 deletions packages/web3-eth/test/integration/defaults.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ describe('defaults', () => {
value: '0x174876e800',
gas: '0x5208',
},
web3Context: eth2 as Web3Context<any>,
web3Context: eth2 as Web3Context,
});
expect(res.networkId).toBe(4);

Expand All @@ -641,7 +641,7 @@ describe('defaults', () => {
gas: '0x5208',
networkId: 5,
},
web3Context: eth2 as Web3Context<any>,
web3Context: eth2 as Web3Context,
});

expect(resWithPassNetworkId.networkId).toBe(BigInt(5));
Expand Down Expand Up @@ -671,7 +671,7 @@ describe('defaults', () => {
value: '0x174876e800',
gas: '0x5208',
},
web3Context: eth2 as Web3Context<any>,
web3Context: eth2 as Web3Context,
});
expect(res.chain).toBe('rinkeby');
});
Expand Down
3 changes: 1 addition & 2 deletions packages/web3-eth/test/integration/web3_eth/send_signed_transaction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,13 @@ along with web3.js. If not, see <http://www.gnu.org/licenses/>.
import { Bytes, SignedTransactionInfoAPI, Transaction } from 'web3-types';
import { DEFAULT_RETURN_FORMAT, FMT_BYTES, FMT_NUMBER, format, hexToNumber } from 'web3-utils';
import { isHexStrict } from 'web3-validator';
import { Web3Eth, InternalTransaction } from '../../../src';
import { Web3Eth, InternalTransaction, transactionSchema } from '../../../src';
import {
closeOpenConnection,
createTempAccount,
getSystemTestProvider,
} from '../../fixtures/system_test_utils';
import { getTransactionGasPricing } from '../../../src/utils/get_transaction_gas_pricing';
import { transactionSchema } from '../../../src';

const HEX_NUMBER_DATA_FORMAT = { bytes: FMT_BYTES.HEX, number: FMT_NUMBER.HEX } as const;

Expand Down
2 changes: 1 addition & 1 deletion packages/web3-plugin-example/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ declare module 'web3' {
}
}

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

protected readonly _contract: Contract<typeof AggregatorV3InterfaceABI>;
Expand Down
13 changes: 7 additions & 6 deletions packages/web3-types/src/web3_api_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@ along with web3.js. If not, see <http://www.gnu.org/licenses/>.

import { JsonRpcId, JsonRpcIdentifier } from './json_rpc_types';

export type Web3APISpec = Record<string, (...params: any) => any>;
export type Web3APIMethod<T extends Web3APISpec> = string & keyof T;
export type Web3APIParams<API extends Web3APISpec, Method extends Web3APIMethod<API>> = Parameters<
API[Method]
>;
export type Web3APISpec = Record<string, (...params: any) => any> | any;
export type Web3APIMethod<T extends Web3APISpec> = string & keyof Exclude<T, any>;
export type Web3APIParams<
API extends Web3APISpec,
Method extends Web3APIMethod<API>,
> = API extends Record<string, (...params: any) => any> ? Parameters<API[Method]> : any;

export interface Web3APIRequest<API extends Web3APISpec, Method extends Web3APIMethod<API>> {
method: Method;
Expand All @@ -37,4 +38,4 @@ export interface Web3APIPayload<API extends Web3APISpec, Method extends Web3APIM
export type Web3APIReturnType<
API extends Web3APISpec,
Method extends Web3APIMethod<API>,
> = ReturnType<API[Method]>;
> = API extends Record<string, (...params: any) => any> ? ReturnType<API[Method]> : any;
4 changes: 2 additions & 2 deletions scripts/system_tests_utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ const maxNumberOfAttempts = 10;
const intervalTime = 5000; // ms

export const waitForOpenConnection = async (
web3Context: Web3Context<any>,
web3Context: Web3Context,
currentAttempt = 1,
status = 'connected',
) =>
Expand All @@ -106,7 +106,7 @@ export const waitForOpenConnection = async (
}, intervalTime);
});

export const closeOpenConnection = async (web3Context: Web3Context<any>) => {
export const closeOpenConnection = async (web3Context: Web3Context) => {
if (!isWs && !isIpc) {
return;
}
Expand Down