Initial Multi-chain implementation #180

hcho112 · 2024-03-04T04:26:05Z

DO NOT MERGE, this PR is currently created for review/discussion with the maintainers.
This has been implemented along with:
https://gist.github.com/esaminu/8ecfd8524a3404d34135acde9c7cbb8b
https://gist.github.com/esaminu/f8cc37849de754f228c5a67bebce9b0f

Current out put

Todo:

Integrate with near-fastauth-wallet and use it against real world example
Update Infura account API
Any UI/UX improvements feedback
Check if gas fee calculation is correct. (It seems too low, but couldn't figure out what went wrong..)

Feel free to leave comments on things that are missing.

vercel · 2024-03-04T04:26:11Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
fast-auth-signer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 17, 2024 11:32pm

esaminu · 2024-03-07T00:50:49Z

packages/near-fast-auth-signer/src/components/AuthCallback/AuthCallback.tsx

@@ -40,11 +41,14 @@ const onCreateAccount = async ({
  email,
  gateway,
 }) => {
+  // Stop from LAK with multi-chain contract
+  const containBlacklistedContractId = getMultiChainContract() === contract_id;


IMO we should throw earlier i.e. in the create account and add device pages to save the user the trouble. It should throw only if multichain contract is explicitly passed. If no value is passed we can continue the flow and only authenticate the signer

esaminu · 2024-03-07T00:54:09Z

packages/near-fast-auth-signer/src/components/SignMultichain/SignMultichain.tsx

+  });
+
+  const onError = (text: string) => {
+    window.parent.postMessage({ signedDelegates: '', text }, '*');


this is a different payload than the error on line 47. Can we standardize and also would be useful if you documented the possible requests and responses in the PR or on a README on the folder

I will try to make it similar withn SignMultichain component, but convention is already diverged on everywhere else in this repo. (eg. Existing Sign component)

Created issue for handling above issue across whole repo #183
Should be handled separately later

esaminu · 2024-03-07T00:57:15Z

packages/near-fast-auth-signer/src/components/SignMultichain/SignMultichain.tsx

+    };
+
+    window.addEventListener(
+      'multichain-message',


should just be a message event

The reason why I gave different name is because on RpcRoute component handles add/remove event listener. Initially I thought I want to handle it separately, but actually its better to keep the same name, so will change this to 'message'

esaminu · 2024-03-07T00:58:24Z

packages/near-fast-auth-signer/src/components/SignMultichain/SignMultichain.tsx

+  useEffect(() => {
+    const handleMessage = (event) => {
+      console.log('event', event);
+      // Maybe add origin check here? if url of origin is available, maybe update below


yes we should add a check for event.origin and compare with domain in the derivation path as part of the validation. If it's the same we can just proceed without prompting the user

That check is actually handled in here: https://github.com/near/fast-auth-signer/pull/180/files#diff-d0f29bd93950dbf2e4531d2a1c32924daf12c3b34a972bd8b490bf032c283eabR139

esaminu · 2024-03-07T01:00:48Z

packages/near-fast-auth-signer/src/components/SignMultichain/utils.ts

+  const newPublicKeyPoint = oldPublicKeyPoint.add(scalarTimesG);
+
+  return `04${
+    newPublicKeyPoint.getX().toString('hex')


please update this with https://pagodaplatform.slack.com/archives/C04HWL4BJKE/p1709672855903079?thread_ts=1709638154.085769&cid=C04HWL4BJKE

Pessina · 2024-03-12T20:10:33Z

packages/near-fast-auth-signer/src/components/CreateAccount/CreateAccount.tsx

    const methodNames = searchParams.get('methodNames');
+    const contract_id = searchParams.get('contract_id');


This is duplicated, no? You are also defining it at line 123

Pessina · 2024-03-12T20:12:03Z

packages/near-fast-auth-signer/src/components/CreateAccount/CreateAccount.tsx

@@ -119,6 +120,16 @@ function CreateAccount() {
  const [searchParams] = useSearchParams();
  const [inFlight, setInFlight] = useState(false);

+  const contractId = searchParams.get('contract_id');
+  useEffect(() => {


This is duplicated, isn't better create a custom hook for it?

Or call it only from the App.ts? (you set it to run only for routes that aren't /multi-chain)

Pessina · 2024-03-12T20:12:41Z

packages/near-fast-auth-signer/src/components/AddDevice/AddDevice.tsx

    const methodNames = searchParams.get('methodNames');
+    const contract_id = searchParams.get('contract_id');


Duplicated, declared above too, no?

Pessina · 2024-03-12T20:15:58Z

packages/near-fast-auth-signer/src/components/SignMultichain/MultichainExecutor.ts

@@ -0,0 +1,33 @@
+import { EVMInterface, BTCInterface, MultichainExecutionResponse, InitializationConfig } from './types';


I think you are already planning it, but just to not. This can be removed

…eys without accountId

…e Sign

Pessina · 2024-03-15T20:42:13Z

packages/near-fast-auth-signer/src/components/SignMultichain/SignMultichain.tsx

+        });
+        setInFlight(false);
+        if (response.success) {
+          window.parent.postMessage({ type: 'response', message: `Successfully sign and send transaction, ${response.transactionHash}` }, '*');


I think you should post the transaction hash, so the parent can check it's status, results or whatever it needs to do

Pessina · 2024-03-15T20:42:41Z

packages/near-fast-auth-signer/src/components/SignMultichain/SignMultichain.tsx

+        if (response.success) {
+          window.parent.postMessage({ type: 'response', message: `Successfully sign and send transaction, ${response.transactionHash}` }, '*');
+        } else {
+          // @ts-ignore


Why do we need @ts-ignore here?

Pessina · 2024-03-15T20:43:14Z

packages/near-fast-auth-signer/src/components/SignMultichain/SignMultichain.tsx

+          onError(response?.errorMessage);
+        }
+      } catch (e) {
+        setInFlight(false);


This should go on the finnaly block, no?

Pessina · 2024-03-15T20:49:48Z

packages/near-fast-auth-signer/src/components/SignMultichain/SignMultichain.tsx

+  }, [deserializedDerivationPath?.asset, deserializedDerivationPath?.domain, isValid, message]);
+
+  // If domain info passed from derivation path is same as window.parent.orgin, post message to parent directly
+  useEffect(() => {


I believe you can use a single useEffect on this page where it performs

setEventListener => receiveMessage => validateMessage => signMultiChain

With that you optimize the code and make it more easy to read.

Pessina · 2024-03-15T20:53:52Z

packages/near-fast-auth-signer/src/components/SignMultichain/SignMultichain.tsx

+
+    try {
+      if (amountInfo.tokenAmount === 0 && message) {
+        setInFlight(true);


The setInFlight should only be managed on the top level functions.

Doing something like

const topLevel = async () => { setInFlight(true) try { await processMessage(); } catch (e) { onError(e); } finally { setInFlight(false); } };

Inside the processMessage you don't need to setInFlight, you only throw the error if necessary

The process mesage will be the onConfirm and the eventListener

Pessina · 2024-03-15T21:13:18Z

packages/near-fast-auth-signer/src/components/SignMultichain/schema.ts

+import { BorshSchema } from 'borsher';
+
+export const derivationPathSchema = BorshSchema.Struct({
+  asset:         BorshSchema.String,


I'm not sure if it should be a string.

I think it's better create a enum of supported chains and re-use it on all the places where you do chain checks.

Pessina · 2024-03-15T21:15:56Z

packages/near-fast-auth-signer/src/components/SignMultichain/utils.ts

+    const res = await getMultichainCoinGeckoPrice(asset);
+    if (!res) {
+      return {
+        price:       0,


If you can't get the data, I think it's better to use null/undefined so it's clear for the caller that the data couldn't be fetched. 0 still a number

Pessina · 2024-03-15T21:21:49Z

packages/near-fast-auth-signer/src/components/SignMultichain/utils.ts

+
+export const getMultichainCoinGeckoPrice = async (asset: DerivationPathDeserialized['asset']) => fetchGeckoPrices(multichainAssetToCoinGeckoId(asset));
+
+const convertTokenToReadable = (value : MultichainInterface['value'], asset: DerivationPathDeserialized['asset']) => {


Isn't better getTokenAmount() I think it's more readable and down there you do

const tokenAmount = convertTokenToReadable()

I think it sounds more readable

Pessina · 2024-03-15T21:22:19Z

packages/near-fast-auth-signer/src/components/SignMultichain/utils.ts

+    };
+  }
+  return {
+    price:       0,


Same as above

Pessina · 2024-03-15T21:24:13Z

packages/near-fast-auth-signer/src/components/SignMultichain/utils.ts

+  };
+};
+
+export const shortenAddress = (address: string): string => {


There are some duplicated numbers, is this a bug? I think the user would expect the two parts of the string to be not overlapping.

That's what I would expect

…posal Refactor SignMultichain component and improve type checking

hcho112 requested review from esaminu and Pessina March 4, 2024 04:26

vercel bot deployed to Preview March 4, 2024 05:13 View deployment

hcho112 force-pushed the multi-chain-singer branch from f6f2d30 to 105741d Compare March 5, 2024 04:32

vercel bot deployed to Preview March 5, 2024 04:33 View deployment

vercel bot deployed to Preview March 5, 2024 21:30 View deployment

esaminu reviewed Mar 7, 2024

View reviewed changes

vercel bot deployed to Preview March 7, 2024 06:41 View deployment

vercel bot deployed to Preview March 7, 2024 06:42 View deployment

vercel bot deployed to Preview March 8, 2024 02:03 View deployment

vercel bot deployed to Preview March 8, 2024 06:36 View deployment

vercel bot deployed to Preview March 11, 2024 07:36 View deployment

Pessina requested changes Mar 12, 2024

View reviewed changes

hcho112 added 12 commits March 13, 2024 11:12

chore: change eslint config to support BigInt syntax

de32608

feat: install few packages for supporting multi-chain with ethereum

3a5db4f

fix: supporting email format that contains + sign

fcf5eee

feat: modules install

683aa18

fix: remove extra curly bracket

ddd570c

feat: implement logic to avoid creating LAK with multichain contract id

4054ed0

feat: improve logic on getCorrectAccessKey that allow to get access k…

3308b20

…eys without accountId

feat: initialize firestore early, remove invalid accountid

90ce977

feat: initial multichain implementation

e269a6f

chore: lint fixes

45210ef

fix: update the SignMultichain component to use same getAuthState lik…

1fe9971

…e Sign

fix: update useIframeDialogConfig logic

eaa683b

feat: update with new APIs

fc9c504

hcho112 force-pushed the multi-chain-singer branch from 6f8a6f7 to fc9c504 Compare March 15, 2024 03:15

vercel bot deployed to Preview March 15, 2024 03:16 View deployment

chore: added lint ignore on test code

817d63f

vercel bot deployed to Preview March 15, 2024 03:52 View deployment

Pessina requested changes Mar 15, 2024

View reviewed changes

Pessina added 16 commits March 16, 2024 02:37

Replace 3 useEffect with 1

c2713c1

Improve Chains type checking

faae496

BTC working

411e149

Fix BTC units

6f1f0b5

Fix crypto conversion and add proper types to args

89b2ef0

Clean up testcode

15ebf40

Add transaction as args to signMultichainTransaction

011fa52

Fix loading issue

d16637d

Introduce re-usable hook for contract filtering

b311839

Add TODO and docs

9141a0f

Add security todo

21073ca

Fix URL encoding issue and disabling button

bb133db

Disable button when firebase is loading

ca54fcf

Encode email on login page

ef4cce3

Only allow multi chain route on testnet

c2f3a98

Merge pull request #193 from near/multi-chain-signer-improvements-pro…

8ec1a87

…posal Refactor SignMultichain component and improve type checking

Pessina previously approved these changes Mar 17, 2024

View reviewed changes

vercel bot deployed to Preview March 17, 2024 23:24 View deployment

Run BTC sign sequentially

00742de

Pessina dismissed their stale review via 00742de March 17, 2024 23:31

vercel bot deployed to Preview March 17, 2024 23:32 View deployment

Pessina approved these changes Mar 17, 2024

View reviewed changes

hcho112 merged commit b74a06c into main Mar 17, 2024
5 of 6 checks passed

Pessina deleted the multi-chain-singer branch May 31, 2024 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Multi-chain implementation #180

Initial Multi-chain implementation #180

hcho112 commented Mar 4, 2024 •

edited

Loading

vercel bot commented Mar 4, 2024 •

edited

Loading

esaminu Mar 7, 2024

esaminu Mar 7, 2024

hcho112 Mar 7, 2024

hcho112 Mar 7, 2024

esaminu Mar 7, 2024

hcho112 Mar 7, 2024

esaminu Mar 7, 2024

hcho112 Mar 7, 2024

esaminu Mar 7, 2024

Pessina Mar 12, 2024

Pessina Mar 12, 2024

Pessina Mar 12, 2024

Pessina Mar 12, 2024

Pessina Mar 15, 2024

Pessina Mar 15, 2024

Pessina Mar 15, 2024

Pessina Mar 15, 2024

Pessina Mar 15, 2024

Pessina Mar 15, 2024

Pessina Mar 15, 2024

Pessina Mar 15, 2024

Pessina Mar 15, 2024

Pessina Mar 15, 2024

		const methodNames = searchParams.get('methodNames');
		const contract_id = searchParams.get('contract_id');

		@@ -0,0 +1,33 @@
		import { EVMInterface, BTCInterface, MultichainExecutionResponse, InitializationConfig } from './types';


		export const getMultichainCoinGeckoPrice = async (asset: DerivationPathDeserialized['asset']) => fetchGeckoPrices(multichainAssetToCoinGeckoId(asset));

		const convertTokenToReadable = (value : MultichainInterface['value'], asset: DerivationPathDeserialized['asset']) => {

Initial Multi-chain implementation #180

Initial Multi-chain implementation #180

Conversation

hcho112 commented Mar 4, 2024 • edited Loading

vercel bot commented Mar 4, 2024 • edited Loading

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

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

hcho112 commented Mar 4, 2024 •

edited

Loading

vercel bot commented Mar 4, 2024 •

edited

Loading