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

feat: support custom networks from config service [SW-136] #4054

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from

Conversation

jmealy
Copy link
Contributor

@jmealy jmealy commented Aug 8, 2024

What it solves

Resolves: https://www.notion.so/safe-global/76f265123c0a4ad680947ce489804342?v=17c30f9503cd4d48a622209408de56d6&p=e7fed2501e9e4650b30ba9b4f618a804&pm=s

Relevant changes to client-gateway: safe-global/safe-client-gateway#1752

How this PR fixes it

  • If there are safe contract addresses set for a network in the config service, use them to initialize the protocol-kit.
  • Only applies when the app is run as a fork, or in dev/staging. Does not apply to the production app.

How to test it

  • Add custom contract addresses to a chain on the config service. The chain should not exist in safe-deployments, to make sure that the addresses from the config service are used.
  • Test that safe creation works on that chain and that the safe is functional.

Screenshots

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

Copy link

Copy link

github-actions bot commented Aug 8, 2024

Copy link

github-actions bot commented Aug 8, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link

github-actions bot commented Aug 8, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
78.56% (-0.01% 🔻)
11770/14983
🔴 Branches
59.11% (-0.01% 🔻)
3003/5080
🟡 Functions
66.02% (-0.02% 🔻)
1879/2846
🟡 Lines
79.95% (-0.01% 🔻)
10614/13276
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟡
... / useCustomNetworkContracts.ts
61.9% 55.56% 50% 62.5%

Test suite run success

1464 tests passing in 201 suites.

Report generated by 🧪jest coverage report action from 75e4694

Copy link

github-actions bot commented Aug 14, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link

github-actions bot commented Aug 14, 2024

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 945.38 KB (🟡 +332 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Two Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/new-safe/advanced-create 35.17 KB (🟡 +77 B) 980.54 KB
/new-safe/create 34.39 KB (🟡 +42 B) 979.77 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@jmealy jmealy requested a review from katspaugh August 14, 2024 11:52
@jmealy jmealy marked this pull request as ready for review August 14, 2024 11:52
Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@jmealy jmealy requested a review from gjeanmart August 21, 2024 08:02
Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

const mockSafe = {} as Safe
const initMock = jest.spyOn(coreSDK, 'initSafeSDK').mockReturnValue(Promise.resolve(mockSafe))
const setSDKMock = jest.spyOn(coreSDK, 'setSafeSDK')
// it('initializes a Core SDK instance', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

You can use xit to comment out a test. Why is it commented out though?

import { IS_OFFICIAL_HOST, IS_PRODUCTION } from '@/config/constants'

const isContractNetworkConfig = (obj: Omit<ChainInfo['contractAddresses'], 'safeWebAuthnSignerFactoryAddress'>) => {
return Object.values(obj).every((value) => !!value)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a replacement for _.isEmpty()?

The function name is isContractNetworkConfig but all this function does is check if all object's properties are truthy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, the name doesn't make sense 👍

The reason for the check is that if the contracts are not defined in the config service admin, the contractNetworks object comes back with all the properties defined but set to null:

const contractAddresses: {
    safeSingletonAddress:  null;
    ...
}

Comment on lines 14 to 23
export const useCustomNetworkContracts = () => {
const currentChain = useCurrentChain()
if (!currentChain || (IS_PRODUCTION && IS_OFFICIAL_HOST)) return

// Exclude safeWebAuthnSignerFactoryAddress as it is not yet supported.
const { safeWebAuthnSignerFactoryAddress, ...contractAddresses } = currentChain.contractAddresses

if (!isContractNetworkConfig(contractAddresses)) return
return contractAddresses as ContractNetworkConfig
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const useCustomNetworkContracts = () => {
const currentChain = useCurrentChain()
if (!currentChain || (IS_PRODUCTION && IS_OFFICIAL_HOST)) return
// Exclude safeWebAuthnSignerFactoryAddress as it is not yet supported.
const { safeWebAuthnSignerFactoryAddress, ...contractAddresses } = currentChain.contractAddresses
if (!isContractNetworkConfig(contractAddresses)) return
return contractAddresses as ContractNetworkConfig
}
export const useCustomNetworkContracts = IS_PRODUCTION && IS_OFFICIAL_HOST ? () => {
} : () => undefined

Copy link
Contributor Author

@jmealy jmealy Aug 30, 2024

Choose a reason for hiding this comment

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

changed but it should be the opposite way I think:

export const useCustomNetworkContracts = IS_PRODUCTION && IS_OFFICIAL_HOST ? () => undefined  : () => {
...}

@katspaugh katspaugh requested a review from schmanu August 27, 2024 14:43
@katspaugh
Copy link
Member

@schmanu please also review, there might be an overlap with some of the multichain logic.

Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

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.

3 participants