Example Plugin Implementation #5393

spacesailor24 · 2022-08-30T02:14:32Z

Example implementation for #5091

Unresolved Issues

Points of Discussion

~~I believe the Web3APISpec on this line should be ChainlinkPluginAPI, but when making the change the following error appears for this~~
- ~~Attempted fixes for this include: Fix typing compatibility when linking a contract to a context #5416 and Web3Context Generic Fix #5480~~

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)

~~In order to get the declare module to work, I needed to update the Web3 package from using the default export to a named one - I'm not sure if/how this affects anything~~ This commit updates Web3 to be exported using default and named export
- ~~Related to this change, importing Web3 like this is the only way I could get the module augmentation to work~~ The above mentioned commit also updates this to use web3 to augment module instead of web3/dist/web3
~~The return type for getPrice is inferred from the contract ABI instead of ChainlinkPluginAPI~~
- ~~This doesn't matter so much because the type the end user will see when calling web3.chainlink.getPrice is defined by ChainlinkPluginAPI (i.e. user will see getPrice returns Promise<Price>)~~

render · 2022-08-30T02:14:37Z

Your Render PR Server URL is https://web3js-docs-pr-5393.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-cc6n52pgp3jv8lhb6gb0.

…e/web3.js into wyatt/4.x/5091-chainlink-plugin

lgtm-com · 2022-08-30T02:21:08Z

This pull request introduces 1 alert when merging d0d388b into aeceed2 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

packages/web3-plugin-example/test/unit/plugin.test.ts

lgtm-com · 2022-08-30T03:16:23Z

This pull request introduces 1 alert when merging db296ff into aeceed2 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

packages/web3-core/src/web3_context.ts

mpetrunic · 2022-08-30T08:16:31Z

packages/web3-plugin-example/package.json

+		"ts-jest": "^28.0.7",
+		"typescript": "^4.7.4"
+	},
+	"dependencies": {


should those be peerDependencies?

I'm not sure they can be since the plugin is actually importing the packages?

import { ContractAbi } from 'web3-eth-abi'; import Contract from 'web3-eth-contract'; import { Web3PluginBase } from 'web3-core'; import { Address, Web3APISpec } from 'web3-types'; // @ts-expect-error 'Web3' is declared but its value is never read. import { Web3 } from 'web3/dist/web3';

That doesn't really matter. The only difference here is if you put peer dependencies and set a broad range like ">=4.0.0 < 5" it will warn the user if he is supplying an invalid version, otherwise it would use his version. On the other hand, if you leave it as regular dep, it will install the plugin version of web3 along with a user-supplied version of web3.

This commit adds the dependencies as both peerDependencies and devDependencies as per this

mpetrunic · 2022-08-30T08:45:12Z

packages/web3/src/index.ts

 * This comment _supports3_ [Markdown](https://marked.js.org/)
 */
-import Web3 from './web3';
+import { Web3 } from './web3';


Why not do both module export and named export instead of just module export?
Named export generally works better with IDE (auto-import and type hinting)

In this commit I've updated Web3 to be exported via default and named

mpetrunic · 2022-08-30T08:50:13Z

packages/web3-core/src/web3_context.ts

 	privateKey?: HexString | Buffer;
 }) => Promise<ReturnType>;
+
+export abstract class Web3PluginBase<API extends Web3APISpec> extends Web3Context<API> {


I assume this is how the rest of the modules in web3 are built. The only issue I see with this is once we have plugins that depend on other plugins, types won't work because we are augmenting Web3 class and plugins are using Web3Context.

Alternative of top of my head would be:

export const plugin1 = { namespace, init: (web3: Web3) => Plugin1Api } declare module web3{ interface Web3 { plugin1: Plugin1Api; } }

and if you require plugin1 in plugin2:

import plugin2 from "plugin2" export const plugin1 = { namespace, needs: [plugin1.namespace] // because we imported plugin2, Web3 class here will be augmented with plugin1 api init: (web3: Web3) => Plugin1Api }

#5490 tracks this

mpetrunic · 2022-08-30T08:52:35Z

packages/web3-plugin-example/src/index.ts

+	getPrice: () => Promise<Price>;
+}
+
+declare module 'web3/dist/web3' {


Have you tested if end user imports Web3 from 'web3' (instead of 'web3/dist/web3') will they see augmented API?

After this commit, Web3 is now exported as a named export which means this is now:

import { Web3 } from 'web3'; declare module 'web3'

jdevcs · 2022-08-30T12:11:39Z

packages/web3/src/web3.ts

 import { Web3EthInterface } from './types';
 import { Web3PkgInfo } from './version';

-export default class Web3 extends Web3Context<EthExecutionAPI> {


This will break minified web3.js build #5345

This commit updates Web3 to be exported via default and named. I've created a test example that uses the web3.min.js and calls various functions to ensure the minified build works

packages/web3-plugin-example/package.json

packages/web3-plugin-example/webpack.config.js

jdevcs

If we want to keep this as example for others to implement plugins, :
web3-plugin-example example package should be placed under tools

jdevcs · 2022-08-30T14:45:51Z

packages/web3-plugin-example/src/index.ts

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


We should leverage context coming via inheritance Web3PluginBase and use that to instantiate contract. Ideally with every usage there should not be need to check :
if (this._contract.currentProvider === undefined) this._contract.link(this); if context is used while instantiation.

#5492 tracks this and there seems to be a bug with updating a context that was passed upon instantiation of Contract

lgtm-com · 2022-08-30T17:27:55Z

This pull request introduces 1 alert when merging a2f43db into e2afb75 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

Muhammad-Altabba

Looks great beside tiny stuff in addition to the already mentioned points by other reviewers.

tools/web3-plugin-example/CHANGELOG.md

packages/web3-core/CHANGELOG.md

packages/web3-types/CHANGELOG.md

…e/web3.js into wyatt/4.x/5091-chainlink-plugin

CHANGELOG.md

packages/web3-types/CHANGELOG.md

Co-authored-by: Muhammad Altabba <[email protected]>

CHANGELOG.md

…e/web3.js into wyatt/4.x/5091-chainlink-plugin

CHANGELOG.md

packages/web3/CHANGELOG.md

Co-authored-by: Muhammad Altabba <[email protected]>

Muhammad-Altabba · 2022-10-24T20:10:27Z

tools/web3-plugin-example/test/unit/contract_method_wrappers.test.ts

+				'0xc41b9a4f654c44552e135f770945916f57c069b80326f9a5f843e613491ab6b1';
+
+			requestManagerSendSpy.mockResolvedValueOnce(expectedGasPrice);
+			// Not sure what's being mocked here


Could we, please, have an issue for investigating the matter and enhancing this piece of code?

Addressed by this commit

spacesailor24 added 3 commits August 29, 2022 15:36

Init abstract Web3PluginBase class. Init registerPlugin method

2da30e0

Update web3.ts to have a named export instead of using default

530b4ce

Init web3-plugin-example package

546fa28

spacesailor24 added the 4.x 4.0 related label Aug 30, 2022

spacesailor24 requested review from Muhammad-Altabba, avkos, jdevcs, luu-alex, mpetrunic, nazarhussain and nikoulai August 30, 2022 02:14

spacesailor24 self-assigned this Aug 30, 2022

spacesailor24 added 3 commits August 29, 2022 16:14

Merge branch '4.x' into wyatt/4.x/5091-chainlink-plugin

cb636c2

Update CHANGELOG and README

3233a76

Merge branch 'wyatt/4.x/5091-chainlink-plugin' of github.com:ChainSaf…

d0d388b

…e/web3.js into wyatt/4.x/5091-chainlink-plugin

spacesailor24 mentioned this pull request Aug 30, 2022

Research Plug-in base architecture #5091

Closed

spacesailor24 commented Aug 30, 2022

View reviewed changes

packages/web3-plugin-example/test/unit/plugin.test.ts Outdated Show resolved Hide resolved

Update packages/web3-plugin-example/test/unit/plugin.test.ts

db296ff

mpetrunic reviewed Aug 30, 2022

View reviewed changes

jdevcs suggested changes Aug 30, 2022

View reviewed changes

jdevcs reviewed Aug 30, 2022

View reviewed changes

Merge branch '4.x' into wyatt/4.x/5091-chainlink-plugin

a2f43db

spacesailor24 added 3 commits August 30, 2022 09:35

Merge branch '4.x' into wyatt/4.x/5091-chainlink-plugin

45881ee

Remove unneeded npm scripts

6514f13

Remove webpack config

9b4896e

Muhammad-Altabba suggested changes Oct 11, 2022

View reviewed changes

tools/web3-plugin-example/CHANGELOG.md Show resolved Hide resolved

packages/web3-core/CHANGELOG.md Show resolved Hide resolved

spacesailor24 commented Oct 11, 2022

View reviewed changes

packages/web3-types/CHANGELOG.md Outdated Show resolved Hide resolved

spacesailor24 added 3 commits October 11, 2022 06:09

Update packages/web3-types/CHANGELOG.md

051a35f

Add CHANGELOG changes to root CHANGELOG

bba74e6

Merge branch 'wyatt/4.x/5091-chainlink-plugin' of github.com:ChainSaf…

6093e5b

…e/web3.js into wyatt/4.x/5091-chainlink-plugin

Muhammad-Altabba reviewed Oct 12, 2022

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

Muhammad-Altabba reviewed Oct 12, 2022

View reviewed changes

packages/web3-types/CHANGELOG.md Outdated Show resolved Hide resolved

spacesailor24 commented Oct 12, 2022

View reviewed changes

packages/web3-types/CHANGELOG.md Outdated Show resolved Hide resolved

spacesailor24 and others added 2 commits October 12, 2022 11:58

Update packages/web3-types/CHANGELOG.md

9481519

Update CHANGELOG.md

c1b2cc5

Co-authored-by: Muhammad Altabba <[email protected]>

spacesailor24 commented Oct 12, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

spacesailor24 added 6 commits October 12, 2022 12:00

Update CHANGELOG.md

744bfc2

Add web3-plugin-example CHANGELOG

c939cbc

Merge branch 'wyatt/4.x/5091-chainlink-plugin' of github.com:ChainSaf…

e55e65f

…e/web3.js into wyatt/4.x/5091-chainlink-plugin

Remove unnecessary web3 imports

8d90ddd

Revert change to Web3 export

f242b48

Init web3_export_helper.ts

56bf6b7

spacesailor24 requested a review from Muhammad-Altabba October 20, 2022 23:43

Merge branch '4.x' into wyatt/4.x/5091-chainlink-plugin

b79c68b

Muhammad-Altabba reviewed Oct 24, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Muhammad-Altabba reviewed Oct 24, 2022

View reviewed changes

packages/web3/CHANGELOG.md Outdated Show resolved Hide resolved

spacesailor24 and others added 2 commits October 24, 2022 06:54

Update packages/web3/CHANGELOG.md

d1ba3ec

Co-authored-by: Muhammad Altabba <[email protected]>

Update CHANGELOG.md

1e271b9

Co-authored-by: Muhammad Altabba <[email protected]>

Muhammad-Altabba reviewed Oct 24, 2022

View reviewed changes

Muhammad-Altabba approved these changes Oct 24, 2022

View reviewed changes

Update comments for mocking in contract_method_wrappers.test.ts

4375a5d

spacesailor24 merged commit c83a36e into 4.x Oct 25, 2022

spacesailor24 deleted the wyatt/4.x/5091-chainlink-plugin branch October 25, 2022 04:55

jdevcs mentioned this pull request Nov 1, 2022

Release/4.0.1 alpha.1 #5580

Merged

Muhammad-Altabba mentioned this pull request Nov 7, 2022

Suggestion to simplify Chainlink plugin usage by importing and reexporting modules #5596

Closed

2 tasks

Example Plugin Implementation #5393

Example Plugin Implementation #5393

Uh oh!

Conversation

spacesailor24 commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unresolved Issues

Points of Discussion

Uh oh!

render bot commented Aug 30, 2022

Uh oh!

lgtm-com bot commented Aug 30, 2022

Uh oh!

Uh oh!

lgtm-com bot commented Aug 30, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jdevcs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Aug 30, 2022

Uh oh!

Muhammad-Altabba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spacesailor24 commented Aug 30, 2022 •

edited

Loading