PRO-1036: EIP1271 Module #1

Yash-Chaubal · 2023-02-20T01:03:35Z

This module exports two functions: isValidEip1271Signature and isValidEip1271SignatureForAllNetworks - the first one returns true/false, the latter returns an array containing the results for each network.

Important code located in src/utils + src/constants

linear · 2023-02-20T01:03:38Z

PRO-1036 Etherspot npm package to inject EIP1271 support

A ticket to implement poocart's idea of a library which will allow any dapp to add support for EIP1271. More information is available here: https://pillarwallet.slack.com/archives/CEGHL682H/p1675249312222659

arddluma

Good job 🚀 ! Left some comments there. If you don't mind could you please add a readme how to use module ? ty!

arddluma · 2023-02-20T09:31:36Z

src/constants/chain-constants.ts

@@ -0,0 +1,87 @@
+import { NetworkNames } from "etherspot";


Can we just import NetworkNames module instead of install etherspot cuz it is a bit heavy ?

arddluma · 2023-02-20T09:31:54Z

src/constants/chain-constants.ts

+  [CHAIN_ID.XDAI]: {
+    id: CHAIN_ID.XDAI,
+    name: NetworkNames.Xdai,
+    rpcUrl: "https://rpc.gnosischain.com/" + process.env.REACT_APP_INFURA_KEY,


There is no need for Infura key in this case ^

arddluma · 2023-02-20T09:32:45Z

src/constants/chain-constants.ts

+
+type ISupportedNetworks = Record<CHAIN_ID, INetworkData>;
+
+export const supportedNetworks: ISupportedNetworks = {


This is fun until one of the RPC doesn't work ?! Can't the dApp use the existing connect provider? We shouldn't hardcode RPC like this.

we do it same way in contracts (https://github.com/etherspot/etherspot-contracts/blob/master/extensions/constants.ts), but we probably should add an option to pass your own rpc urls

we do it same way in contracts (https://github.com/etherspot/etherspot-contracts/blob/master/extensions/constants.ts), but we probably should add an option to pass your own rpc urls

I agree. The RPC URL must be passed as a parameter to the module.

Got it, I remembered one of the specifications was that we wanted to cycle through each network to see if the sig was valid, so this was the only way I saw a way to do so.

I can change it so that the users themselves provider the rpc/provider, but then there won't really be a way for us to check valid sigs for each network within the module itself (without the user sending us an array of rpcs by network name, but that doesn't seem very user friendly)

@ch4r10t33r @arddluma I've added a new param to accept a provider from the user. If we're happy with this, and have no need to cycle through networks to check if its valid for all, then I'll remove the unneeded code.

@ch4r10t33r @arddluma I've added a new param to accept a provider from the user. If we're happy with this, and have no need to cycle through networks to check if its valid for all, then I'll remove the unneeded code.

If we do this it will be single chain... Would be better to have it multichain instead.
Let's stick with this approach if possible please 🙏
@ch4r10t33r Thoughts?

Well, in theory, this module has to be used specifically for a chain, no? If you wish to initialize for several chains at once, then we have to pass chainId as the key and the rpc-url as value?

arddluma · 2023-02-20T09:35:50Z

package.json

+  },
+  "dependencies": {
+    "@rollup/plugin-json": "^6.0.0",
+    "axios": "^0.27.2",


Do we need axios ?

I would also use specific versions of the dependencies, instead of a generic.

arddluma · 2023-02-20T09:42:13Z

constants/abi.d.ts

@@ -0,0 +1,36 @@
+export declare const abiEip1271Json: {


Do we need this file at all ? I guess it is already in src/contants/abi.ts

I think that's one of the auto-generated files, but I'll see what deleting it does

+1 re @arddluma comment

ch4r10t33r · 2023-02-20T14:54:41Z

@Yash-Chaubal Please can you add a README.md explaining how the module should be used? A simple example will help. Thanks.

poocart

general comment – I think the lib files compilation should happen inside publish pipeline and currently compiled files shouldn't be committed

cc @arddluma

poocart · 2023-02-20T16:59:10Z

src/utils/verify-signature.ts

+  // const validArr = await isValidEip1271SignatureForAllNetworks(walletAddress, hash, signature);
+  // return validArr.filter((item) => !!item.valid).length > 0;


Suggested change

// const validArr = await isValidEip1271SignatureForAllNetworks(walletAddress, hash, signature);

// return validArr.filter((item) => !!item.valid).length > 0;

poocart · 2023-02-20T17:00:20Z

src/constants/chain-constants.ts

@@ -0,0 +1,118 @@
+export enum NetworkNames {


these can be taken from Etherspot SDK, there's constant for that named under same name

poocart · 2023-02-20T17:04:03Z

rollup.config.js

+        exclude: ["./example/**", "./src/test/**"],
+      }),
+      replace({
+        __ETHERSPOT_PROJECT_KEY__: process.env.ETHERSPOT_PROJECT_KEY ?? "",


this is not needed, we're not using Etherspot SDK anywhere here for it's API access

poocart · 2023-02-20T17:11:13Z

src/utils/verify-signature.ts

+  const valid = await checkSignature(walletAddress, hash, signature, provider);
+  return !!valid;


checkSignature returns bool already

Suggested change

const valid = await checkSignature(walletAddress, hash, signature, provider);

return !!valid;

return checkSignature(walletAddress, hash, signature, provider);

poocart · 2023-02-20T19:01:26Z

src/utils/verify-signature.ts

+const isCaseInsensitiveMatch = (val1: string, val2: string) => val1.toLowerCase() === val2.toLowerCase();
+
+const createProvider = (rpcUrl: string) => {
+  const isWss = rpcUrl.substring(0, 3) === "wss";


just an easier suggestion, also wss to ws just in case it's insecure sockets

Suggested change

const isWss = rpcUrl.substring(0, 3) === "wss";

const isWss = rpcUrl.startsWith("ws");

poocart · 2023-02-20T19:03:37Z

src/utils/verify-signature.ts

+  signature: string
+): Promise<boolean> => {
+  const validArr = await isValidEip1271SignatureForAllNetworks(rpcUrls, walletAddress, hash, signature);
+  return validArr.filter((item) => !!item.valid).length > 0;


easier approach, returns on first match

Suggested change

return validArr.filter((item) => !!item.valid).length > 0;

return validArr.some((item) => !!item.valid);

poocart

generally just couple things:

compilation should happen inside publish pipeline therefore compiled files removed? cc @arddluma
same typed abi for eip1271 sits in two places: constants and utils, make sure leave single

otherwise lgtm and well done! 🚀

arddluma

LGTM well done 💯 !
As masta @poocart said, we need to remove compiled files such as index.js and index.js.map

initial push

9a11bfb

removed unnecessary packages+plugins

846b727

Yash-Chaubal requested review from arddluma, IAmKio, MayukhSamadder2000 and poocart February 20, 2023 01:23

arddluma requested changes Feb 20, 2023

View reviewed changes

arddluma requested review from 0xSulpiride and ch4r10t33r February 20, 2023 14:10

Yash-Chaubal added 2 commits February 20, 2023 15:45

added provider param + removed packages

2211ed1

added README.md

a43463b

poocart reviewed Feb 20, 2023

View reviewed changes

accepts rpcurls + checks sigs by providers

ec044b8

Yash-Chaubal requested review from poocart and arddluma February 20, 2023 18:56

poocart reviewed Feb 20, 2023

View reviewed changes

poocart approved these changes Feb 20, 2023

View reviewed changes

changes per feedback

b6c9a26

arddluma approved these changes Feb 21, 2023

View reviewed changes

removed compiled files

c6a37fb

Yash-Chaubal merged commit 0f2d91d into master Feb 21, 2023

Yash-Chaubal deleted the PRO-1036-eip1271_module branch February 21, 2023 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PRO-1036: EIP1271 Module #1

PRO-1036: EIP1271 Module #1

Yash-Chaubal commented Feb 20, 2023 •

edited

Loading

linear bot commented Feb 20, 2023

arddluma left a comment

arddluma Feb 20, 2023

arddluma Feb 20, 2023

arddluma Feb 20, 2023

0xSulpiride Feb 20, 2023

ch4r10t33r Feb 20, 2023

Yash-Chaubal Feb 20, 2023

Yash-Chaubal Feb 20, 2023

arddluma Feb 20, 2023

ch4r10t33r Feb 20, 2023

arddluma Feb 20, 2023

ch4r10t33r Feb 20, 2023

arddluma Feb 20, 2023

Yash-Chaubal Feb 20, 2023

poocart Feb 20, 2023

ch4r10t33r commented Feb 20, 2023

poocart left a comment

poocart Feb 20, 2023

poocart Feb 20, 2023

poocart Feb 20, 2023 •

edited

Loading

poocart Feb 20, 2023

poocart Feb 20, 2023 •

edited

Loading

poocart Feb 20, 2023

poocart left a comment

arddluma left a comment


		type ISupportedNetworks = Record<CHAIN_ID, INetworkData>;

		export const supportedNetworks: ISupportedNetworks = {

		// const validArr = await isValidEip1271SignatureForAllNetworks(walletAddress, hash, signature);
		// return validArr.filter((item) => !!item.valid).length > 0;

		const valid = await checkSignature(walletAddress, hash, signature, provider);
		return !!valid;

	const valid = await checkSignature(walletAddress, hash, signature, provider);
	return !!valid;
	return checkSignature(walletAddress, hash, signature, provider);

	const isWss = rpcUrl.substring(0, 3) === "wss";
	const isWss = rpcUrl.startsWith("ws");

	return validArr.filter((item) => !!item.valid).length > 0;
	return validArr.some((item) => !!item.valid);

PRO-1036: EIP1271 Module #1

PRO-1036: EIP1271 Module #1

Conversation

Yash-Chaubal commented Feb 20, 2023 • edited Loading

linear bot commented Feb 20, 2023

arddluma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ch4r10t33r commented Feb 20, 2023

poocart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poocart Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poocart Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poocart left a comment

Choose a reason for hiding this comment

arddluma left a comment

Choose a reason for hiding this comment

Yash-Chaubal commented Feb 20, 2023 •

edited

Loading

poocart Feb 20, 2023 •

edited

Loading

poocart Feb 20, 2023 •

edited

Loading