Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: migrate to new SDK, to bigint, to viem, to react-query (DRAFT) #543

Draft
wants to merge 203 commits into
base: develop
Choose a base branch
from

Conversation

solidovic
Copy link
Contributor

@solidovic solidovic commented Nov 15, 2024

Description

  1. Migrate from BigNumber to bigint.

  2. Migrate from old SDK to the new SDK.

Operations:

  • stake;
  • wrap;
  • unwrap;
  • withdrawal
    work through the new SDK.

Requests for:

  • balances;
  • limits (stake limits);
  • and other on-chain data
    are taken through the functionality of the new SDK.
  1. Migrate from SWR to 'react-query'.

  2. Removed the typechain package.
    Now *abi.ts files are used (in some cases with too much interface description).

  3. Migrate from ethers to viem and removed packages: ethers, @ethersproject/*.

  4. APR fallback: if ETH-API is not available, then SDK is used.

  5. Unused code has been removed.

  6. There was a reduction in the number of lines of code.

Code review notes

...

Testing notes

The whole widget

Checklist:

  • Checked the changes locally.
  • Created / updated analytics events.
  • Created / updated the technical documentation (README.md / docs / etc.).
  • Affects / requires changes in other services (Matomo / Sentry / CloudFlare / etc.).

@@ -30,6 +30,27 @@ export enum HttpMethod {
PATCH = 'PATCH',
}

export const getFunctionSelector = (func: any): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

please also recheck other parts of this code, might have more viem helpers that we can use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked. seems it all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// Here, we can use it without initializeMetricContractAddresses(),
// as initializeMetricContractAddresses() was already called in pages/api/rpc.ts.
// However, be careful when reusing this code.
const contractName = METRIC_CONTRACT_ADDRESSES?.[chainId]?.[address];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment actual?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

abi: Abi,
methodEncoded: string,
): string | null => {
for (const item of abi) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code reparses the abi on every run, we need some sort of caching for selector->method Name

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be out of scope since older code is like this, pls create task for this

@@ -20,6 +20,7 @@ const collectStartupChecksRPCMetrics = async (
const rpcChecksResults = await getRPCChecks();

if (!rpcChecksResults) {
// You will see this on 'yarn dev'
Copy link
Contributor

Choose a reason for hiding this comment

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

Then disable this for dev env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -64,7 +70,7 @@ export const formatBalance = (
};

export const useFormattedBalance: typeof formatBalance = (
balance = Zero,
balance = ZERO,
Copy link
Contributor

Choose a reason for hiding this comment

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

0n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

enabled: wsteth != null && !!(isL2 ? l2.steth : shares),
staleTime: Infinity,
queryFn: () => {
if (wsteth == null) return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but this creates unnecessary footgun, invariants solve this by being easily detectable.

import { STRATEGY_CONSTANT } from 'consts/react-query-strategies';
import { useLidoSDK } from 'modules/web3';

export const useContractAddress = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can make this hook sync, it will be easier for other code. IMO static addresses are okay for non-critical func.
Also consider checking where this hook is used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that for the moment we can leave it as is. Let's make it synchronous as soon as we need it


export const ONE_stETH = parseEther('1');

export const ZERO = parseEther('0');
Copy link
Contributor

Choose a reason for hiding this comment

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

that just 0n, let's remove this const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

import { applyGasLimitRatio } from 'utils/apply-gas-limit-ratio';
import { useDappStatus, useLidoSDK, ZERO, ESTIMATE_AMOUNT } from 'modules/web3';

const fetchGasLimitETH = async (isL2: boolean, wrap: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why any? can use LidoSDKWrap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

misprint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -1,4 +1,5 @@
export const ETHER = '1e18';
// 1e18
export const ETHER = 1_000_000_000_000_000_000n;
Copy link
Contributor

Choose a reason for hiding this comment

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

parseEther('1')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -1,11 +1,14 @@
import { useCallback } from 'react';
import { useForm } from 'react-hook-form';
import { Address } from 'viem';
Copy link
Contributor

Choose a reason for hiding this comment

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

import type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

queryKey: ['submit-gas-limit', stake.core.chainId],
enabled: !!stake.core && !!stake.core.chainId,
...STRATEGY_CONSTANT,
queryFn: async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we have estimateGas in sdk.stake as method

break;
}
case TransactionCallbackStage.MULTISIG_DONE:
txModalStages.successMultisig();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we DRY this between different TXs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had an idea, but it seems we could lose a lot in code readability. I think this boilerplate code is useful. what do you think about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants