Feature: Multiple deployments functions #684

mmv08 · 2024-07-04T14:12:46Z

This PR is a new API proposal for the functions that support the updated JSON format, which supports multiple deployments per network. Implements #685

The complexity of supporting multiple formats is handled by the utility function findDeployments, which contains a function overload with the deployment format parameter.

The new functions are added alongside the existing API functions but with plural deployments at the end of the function name.

Example function:

export const getProxyFactoryDeployments = (filter?: DeploymentFilter): SingletonDeploymentV2 | undefined => {
  return findDeployment(filter, factoryDeployments, DeploymentFormats.MULTIPLE);
};

My reason for going with this approach:
All the complexity is kept within the internal findDeployment function. Developers do not have to import and understand which format means what and don't have to import the additional DeploymentFilter enum. Type shenanigans are kept to a minimum. Also, the alternative approach below would require refactoring package functions to be defined via the function keyword because arrow functions do not support function overloads properly: https://www.perplexity.ai/search/typescript-arrow-function-over-tit1.0R8Rracx_PAUr1nVw#0

Alternative approach:
Add function overloads to the existing functions like this:

export const getSafeSingletonDeployment = (filter?: DeploymentFilter, format = DeploymentFormats.SINGLETON) => {
  return findDeployment(filter, _safeDeployments, format);
};

…ltiple deployments

nlordell

LGTM.

mmv08 · 2024-07-05T09:37:53Z

I put this on pause because of a conflict with #683. I'll let @remedcu make the final changes to the JSON file structure. Also, in the meantime, we can wait for feedback from @gjeanmart and @katspaugh

mmv08 · 2024-07-08T15:00:43Z

src/__tests__/libs.test.ts


 describe('libs.ts', () => {
  describe('getMultiSendDeployment', () => {
    it('should find the preferred deployment first', () => {
      const result = getMultiSendDeployment();
      expect(result).toMatchObject(MultiSend141);
-      expect(result).not.toBe(MultiSend111);


I removed this because it seemed redundant. The same reasoning for other similar removals

nlordell · 2024-07-09T10:08:59Z

src/__tests__/safes.test.ts

+// @ts-expect-error The function is conditionally exported only if the process.env.NODE_ENV is 'test'. TypeScript will not be able to figure it out.
+import { mapJsonToDeploymentsFormatV2 } from '../utils';


uber-nit: To abstract this away, you could have a src/__tests__/utils.ts file that re-exports this symbol, so you only have to work around and document this TS issue in a single plance, and all tests import from:

Suggested change

// @ts-expect-error The function is conditionally exported only if the process.env.NODE_ENV is 'test'. TypeScript will not be able to figure it out.

import { mapJsonToDeploymentsFormatV2 } from '../utils';

import { mapJsonToDeploymentsFormatV2 } from './utils';

nlordell · 2024-07-09T10:10:13Z

src/handler.ts

 export const getDefaultCallbackHandlerDeployment = (filter?: DeploymentFilter): SingletonDeployment | undefined => {
  return findDeployment(filter, defaultCallbackHandlerDeployments);
 };

+export const getDefaultCallbackHandlerDeployments = (filter?: DeploymentFilter): SingletonDeploymentV2 | undefined => {


Should this also have a NatSpec function (saw you adding them in other places)?

nlordell

LGTM. 2 small things, but approving to reduce back-and-forth:

One suggestion to avoid the long @ts-expect-error explanation in multiple places (but feel free to merge without this suggestion if you don't agree)
One missing function documentation

…1-multiple-deployments-function

mmv08 · 2024-07-10T13:42:18Z

@nlordell @remedcu I pushed some changes after the updated script from Shebin, namely:

exported all the deployments to the deployments.ts file (reasons are outlined in the comments in the file itself)
improved types

nlordell · 2024-07-10T15:51:49Z

src/deployments.ts

+// We use some specific types (like `AddressType`) in the `SingletonDeploymentJSON` type, but the TypeScript cannot infer that from the JSON files.
+// So we need to cast them to `SingletonDeploymentJSON` manually. The casting is valid because we have a test in `__tests__/assets.test.ts` that checks that the JSON files are valid.
+// The arrays are sorted by preference, which at the moment means from the most recent version to the oldest.
+// The arrays are prefixed with an underscore because they are not meant to be imported directly.


This shouldn't affect a bundler's ability to tree-shake right?

Well, that's a very complex question. We do not export an ESM bundle with the best tree shaking support.

https://webpack.js.org/guides/tree-shaking/

We also do not support tree shaking with commonjs by adding helper options. I don't know how this affects tree shaking or whether the lib was previously tree-shakeable.

nlordell · 2024-07-10T15:52:18Z

src/deployments.ts

+const _SIGN_MESSAGE_LIB_DEPLOYMENTS = [SignMessageLib141, SignMessageLib130] as SingletonDeploymentJSON[];
+
+export {
+  _ACCESSOR_DEPLOYMENTS,


Why _ prefixes?

// The arrays are prefixed with an underscore because they are not meant to be imported directly.

src/types.ts

nlordell

LGTM, one suggestion for types (I find the object notation easier to understand than the Record type).

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

mmv08 · 2024-07-12T10:52:47Z

I have just realised that I'm an idiot and I forgot to add documentation 🤦

mmv08 added 2 commits July 3, 2024 17:32

wip v2 function

bc158b5

add functions that return the new deployment format which supports mu…

336dfff

…ltiple deployments

mmv08 requested review from gjeanmart and a team as code owners July 4, 2024 14:12

mmv08 requested review from nlordell, akshay-ap and remedcu July 4, 2024 14:12

implement proper functional overloading

354b3c6

mmv08 force-pushed the feat/v2-multiple-deployments-function branch from 6eb41fe to 354b3c6 Compare July 4, 2024 16:11

nlordell approved these changes Jul 5, 2024

View reviewed changes

mmv08 requested a review from katspaugh July 5, 2024 09:36

nlordell mentioned this pull request Jul 5, 2024

Implement New Accessor Functions for Deploments with Multiple Addresses #685

Closed

mmv08 commented Jul 8, 2024

View reviewed changes

mmv08 requested a review from nlordell July 8, 2024 15:01

mmv08 linked an issue Jul 8, 2024 that may be closed by this pull request

Implement New Accessor Functions for Deploments with Multiple Addresses #685

Closed

nlordell reviewed Jul 9, 2024

View reviewed changes

nlordell approved these changes Jul 9, 2024

View reviewed changes

Merge branch 'main' of github.com:gnosis/safe-deployments into feat/v…

a97b531

…1-multiple-deployments-function

mmv08 force-pushed the feat/v2-multiple-deployments-function branch 2 times, most recently from 9410461 to 1979fde Compare July 10, 2024 13:31

extract deployments, refactor tests, fix types

94cf3a1

mmv08 force-pushed the feat/v2-multiple-deployments-function branch from 1979fde to 94cf3a1 Compare July 10, 2024 13:31

mmv08 requested a review from nlordell July 10, 2024 13:41

nlordell reviewed Jul 10, 2024

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

nlordell reviewed Jul 10, 2024

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

nlordell reviewed Jul 10, 2024

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

nlordell approved these changes Jul 10, 2024

View reviewed changes

mmv08 and others added 3 commits July 11, 2024 11:37

Update src/types.ts

0a14d3b

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

Update src/types.ts

359a09b

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

Update src/types.ts

7634cfc

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

mmv08 force-pushed the feat/v2-multiple-deployments-function branch from 8f70103 to 7634cfc Compare July 11, 2024 09:39

katspaugh approved these changes Jul 12, 2024

View reviewed changes

mmv08 merged commit fca8fe3 into main Jul 12, 2024
1 check passed

mmv08 deleted the feat/v2-multiple-deployments-function branch July 12, 2024 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Multiple deployments functions #684

Feature: Multiple deployments functions #684

mmv08 commented Jul 4, 2024 •

edited

Loading

nlordell left a comment

mmv08 commented Jul 5, 2024

mmv08 Jul 8, 2024 •

edited

Loading

nlordell Jul 9, 2024

nlordell Jul 9, 2024

mmv08 Jul 10, 2024

nlordell left a comment

mmv08 commented Jul 10, 2024

nlordell Jul 10, 2024

mmv08 Jul 11, 2024

nlordell Jul 10, 2024

mmv08 Jul 11, 2024

nlordell left a comment

mmv08 commented Jul 12, 2024

		// @ts-expect-error The function is conditionally exported only if the process.env.NODE_ENV is 'test'. TypeScript will not be able to figure it out.
		import { mapJsonToDeploymentsFormatV2 } from '../utils';

Feature: Multiple deployments functions #684

Feature: Multiple deployments functions #684

Conversation

mmv08 commented Jul 4, 2024 • edited Loading

nlordell left a comment

Choose a reason for hiding this comment

mmv08 commented Jul 5, 2024

mmv08 Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

nlordell Jul 9, 2024

Choose a reason for hiding this comment

nlordell Jul 9, 2024

Choose a reason for hiding this comment

mmv08 Jul 10, 2024

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

mmv08 commented Jul 10, 2024

nlordell Jul 10, 2024

Choose a reason for hiding this comment

mmv08 Jul 11, 2024

Choose a reason for hiding this comment

nlordell Jul 10, 2024

Choose a reason for hiding this comment

mmv08 Jul 11, 2024

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

mmv08 commented Jul 12, 2024

mmv08 commented Jul 4, 2024 •

edited

Loading

mmv08 Jul 8, 2024 •

edited

Loading