SDK: Add core/base files and unit tests #3504

barnjamin · 2023-11-07T13:23:36Z

Initial PR to start bringing the connect-sdk into Wormhole monorepo

See the DESIGN.md file for an overview of the intended structure.

The core/base module provides constants and a convenient way to access them as well as utilities for higher level packages.

SEJeff

All nits, very very nice. Feel free to ignore all of them :)

I did not review the typescript bits indepth or the tests at all. The docs are super nice and the sdk is obviously really nice given the magic behind the scenes that connect gives you.

sdk/js-connect/DESIGN.md

sdk/js-connect/README.md

SEJeff · 2023-12-12T06:36:03Z

sdk/js-connect/core/base/src/constants/chains.ts

+  ["Kujira", 4002],
+  // holy cow, how ugly of a hack is that?! - a chainId that's exclusive to a testnet!
+  ["Sepolia", 10002],
+] as const satisfies MapLevel<string, number>;


Does this mean we are going to have yet another place to maintain this list?

Thinking outloud, does it make sense to have a clearly namespaced singular wormhole typescript lib that is solely the constants that can be imported by this new sdk and the "legacy" sdk, or is the legacy one going to be fully deprecated?

I'm hopeful we end up deriving the "legacy" config/consts from this one so we only have to update it/reference it from one spot.

If we do have a single Wormhole ts lib, this one is its own module so should be importable to access the config

the plan is that this SDK is the one to rule them all. the legacy one in the monorepo, as well as the sdk directory inside wormhole-connect, will both be replaced by this.

SEJeff · 2023-12-12T06:40:30Z

testing/Dockerfile.connectsdk.test

+RUN apt-get update && apt-get -y install \
+  git python3 make curl netcat


Suggested change

RUN apt-get update && apt-get -y install \

git python3 make curl netcat

RUN apt-get update && apt-get -y install --no-install-recommends \

git python3 make curl netcat && \

apt-get clean && \

rm -rf /var/lib/apt/lists/*

Put those images on a diet since you're not using a multi-stage build! Feel free to confirm the before and after with this trick:

docker image ls --format "{{ .Size }}" $IMAGE_NAME

testing/connectsdk.sh

Co-authored-by: Jeff Schroeder <[email protected]>

artursapek · 2024-01-04T17:56:55Z

sdk/js-connect/core/base/src/utils/metaprogramming.ts

@@ -0,0 +1,59 @@
+export type Extends<T, U> = [T] extends [U] ? true : false;
+
+export type Function<P extends any[] = any[], R = any> = (...args: P) => R;


doesn't typescript already have a Function built-in?

it does https://www.typescriptlang.org/docs/handbook/2/functions.html#function

cc @nonergodic do we need this definition?

As the documentation says:

This is an untyped function call and is generally best avoided because of the unsafe any return type.

I.e. another thing that TypeScript has and says "oh, it's there but, you shouldn't really use it. Think of it like a rake that we put in tall grass so you can accidentally step on it and have it smack you in the face. We put it there because we thought it was funny."

Ultimately, both Function and Extends, just like e.g. RoArray, are simply there to make stuff more readable (in my book - reasonable people can disagree here I think 🤷‍♂️ )

The fact that Extends compares clothed types might be surprising, so I'm less sure about that one and to what extent it is really mostly a leftover from my very early Typescript days, so that one is perhaps less justified than Function.

Overall, I don't have a strong opinion here.

artursapek · 2024-01-04T18:10:44Z

sdk/js-connect/core/base/src/constants/contracts/relayer.ts

+
+export const relayerContracts = [[
+  "Mainnet", [
+    ["Ethereum",  "0x27428DD2d3DD32A4D7f7C497eAaa23130d894911"],


I'm guessing you copy pastad all this and they don't need to be individually verified?

yes copy/pasta'd but probably makes sense to check a few

artursapek · 2024-01-04T18:13:07Z

I would change the js-connect directory name to typescript or js-v2 or something like that. make it clear this is more than just a Connect SDK.

kev1n-peters · 2024-01-04T18:31:25Z

I think that we should add a .prettierrc file in the root directory and potentially a CI job that enforces TypeScript formatting

nonergodic · 2024-01-04T20:29:02Z

I strongly object to the prettier. What's currently not pretty? I can point to loads of places that will be less readable, once a "prettier" is done with disfiguring the code that I wrote.

nonergodic · 2024-01-04T20:37:02Z

The compromise that Ben and I originally came up with was that "my" part of the code would be exempt (i.e. get a prettier ignore entry). This way, whoever has the most skin in the games in various parts of the code gets to call the shots, rather than having some external "authority" come up with "sensible regulations". Uniformity and standardization is necessary when you run a fast-food franchise, but you'll not get a five star restaurant that way.

barnjamin · 2024-01-04T20:46:39Z

The compromise that Ben and I originally came up with was that "my" part of the code would be exempt (i.e. get a prettier ignore entry). This way, whoever has the most skin in the games in various parts of the code gets to call the shots, rather than having some external "authority" come up with "sensible regulations". Uniformity and standardization is necessary when you run a fast-food franchise, but you'll not get a five star restaurant that way.

fwiw I think I borked the formatting again, will have to make it pretty once again. Can we use the formatting everywhere but cordon off certain things like const declarations with an ignore directive https://prettier.io/docs/en/ignore.html#javascript

nonergodic · 2024-01-04T22:22:31Z

fwiw I think I borked the formatting again, will have to make it pretty once again. Can we use the formatting everywhere but cordon off certain things like const declarations with an ignore directive https://prettier.io/docs/en/ignore.html#javascript

Making my point for me.

Example taken from mapping.ts:
Before "prettier":

type TransformMapping<M extends MappingEntries, S extends Shape | void = void> =
  //check that M has a valid structure for mapping entries
  CartesianRightRecursive<M> extends infer CRR extends RoArray2D
  ? IsRectangular<CRR> extends true
    //ensure CRR is not empty  
    ? CRR extends readonly [RoArray, ...RoArray2D]
      ? S extends Shape
        ? SplitAndReorderKeyValueColumns<CRR, S> extends [
            infer KC extends CartesianSet<MappableKey>,
            infer VC extends CartesianSet
          ]
          ? KC["length"] extends Depth[number]
            ? CombineValueColumnsToLeafValues<VC> extends infer VR extends RoArray<LeafValue>
              ? ProcessNextKeyColmn<KC, VR> extends infer TM extends MappingEntries
                ? [MaybeUnwrapValuesIfAllAreSingletons<TM, KC["length"]>, KC["length"]]
                : never
              : never
            : never
          : never
        //if we don't have an explicit shape, take the first row and subtract 1 (for the value
        //  column) to determine the count of key columns
        : CRR[0] extends readonly [...infer KC extends RoArray, unknown]
        ? KC["length"] extends Depth[number]
          ? [M, KC["length"]]
          : never
        : never
      : never
    : never
  : never;

After "prettier":

type TransformMapping<M extends MappingEntries, S extends Shape | void = void> =
  //check that M has a valid structure for mapping entries
  CartesianRightRecursive<M> extends infer CRR extends RoArray2D
  ? IsRectangular<CRR> extends true
  //ensure CRR is not empty  
  ? CRR extends readonly [RoArray, ...RoArray2D]
  ? S extends Shape
  ? SplitAndReorderKeyValueColumns<CRR, S> extends [
    infer KC extends CartesianSet<MappableKey>,
    infer VC extends CartesianSet
  ]
  ? KC["length"] extends Depth[number]
  ? CombineValueColumnsToLeafValues<VC> extends infer VR extends RoArray<LeafValue>
  ? ProcessNextKeyColmn<KC, VR> extends infer TM extends MappingEntries
  ? [MaybeUnwrapValuesIfAllAreSingletons<TM, KC["length"]>, KC["length"]]
  : never
  : never
  : never
  : never
  //if we don't have an explicit shape, take the first row and subtract 1 (for the value
  //  column) to determine the count of key columns
  : CRR[0] extends readonly [...infer KC extends RoArray, unknown]
  ? KC["length"] extends Depth[number]
  ? [M, KC["length"]]
  : never
  : never
  : never
  : never
  : never;

I rest my case.

kev1n-peters · 2024-01-04T22:52:18Z

Ah, prettier does make the code pretty unreadable in that case. Maybe we could compromise like previously suggested and ignore formatting in cases where it hurts readability.

artursapek · 2024-01-04T23:04:41Z

it's worth trying this prettier option: https://prettier.io/docs/en/options.html#experimental-ternaries

and just generally seeing if we can configure it to suit your fancy. it's highly configurable.

nonergodic · 2024-01-04T23:25:31Z

it's worth trying this prettier option: https://prettier.io/docs/en/options.html#experimental-ternaries

and just generally seeing if we can configure it to suit your fancy. it's highly configurable.

We have a fundamental, philosophical difference here.

You can't nail down what makes readable code, just like you can't nail down virtue and what it means to be virtuous.

Sure, you can give guidelines (be honest, be charitable, and so on) but you can't put this "into code". Trying to replace wisdom with intelligence is exactly the sort of trap that intelligent nerds fall into (I used to be one of them). I now reject this entire line of thinking.

You can define some approximation that you keep refining over time, but I don't see the purpose.

Be a virtuous person. Write readable code.

artursapek · 2024-01-05T15:16:54Z

I don't really see how it's a matter of intelligence or wisdom. I think people who use formatters usually do it to be more efficient. I don't like spending my time formatting my code. it's one of many chores in my life which I prefer to automate as much as I can.

In larger orgs, it's a PITA to make sure everyone has the same tab settings, etc. and diffs become very noisy if you don't. this was why Golang shipped with gofmt from the very beginning; Google was trying to make a good systems language for massive development projects with many contributors.

artursapek · 2024-01-05T15:42:45Z

sdk/js-connect/core/base/src/utils/layout/index.ts

@@ -0,0 +1,36 @@
+export {


this utils/layout is quite a piece of work. I don't see unit tests in this directory. is this unit tested somehow elsewhere in this large diff?

Some tests here https://github.com/wormhole-foundation/wormhole/blob/3926dc88eba6d3e9ede3eed4864796a99a9249fc/sdk/js-connect/core/base/__tests__/layout.ts

artursapek

let's light this candle

barnjamin requested review from a5-pickle, evan-gray, bruce-riley, kev1n-peters and SEJeff as code owners November 7, 2023 13:23

barnjamin force-pushed the connect-sdk/base branch from 6617552 to 4f1652f Compare November 7, 2023 17:22

kcsongor self-requested a review November 7, 2023 17:31

barnjamin force-pushed the connect-sdk/base branch from 4f1652f to c31a8a5 Compare November 14, 2023 18:47

SEJeff previously approved these changes Dec 12, 2023

View reviewed changes

barnjamin dismissed SEJeff’s stale review via 1e3f6b9 December 13, 2023 13:16

barnjamin force-pushed the connect-sdk/base branch from ba1f7ab to 2f002b2 Compare December 13, 2023 13:24

barnjamin and others added 7 commits January 4, 2024 11:59

Add core/base to new sdk directory

b59e4ee

add design, readme and config for CI

4910f73

update with reformated consts and native chain ids

2ae386c

add test files

82d0f48

Update sdk/js-connect/DESIGN.md

65253c6

Co-authored-by: Jeff Schroeder <[email protected]>

Apply suggestions from code review

8787f3c

Co-authored-by: Jeff Schroeder <[email protected]>

Apply suggestions from code review

7379c4a

Co-authored-by: Jeff Schroeder <[email protected]>

barnjamin force-pushed the connect-sdk/base branch 2 times, most recently from a3b8268 to 13225f7 Compare January 4, 2024 17:11

update to latest versions

3926dc8

barnjamin force-pushed the connect-sdk/base branch from 13225f7 to 3926dc8 Compare January 4, 2024 17:19

artursapek reviewed Jan 4, 2024

View reviewed changes

artursapek reviewed Jan 5, 2024

View reviewed changes

artursapek approved these changes Jan 9, 2024

View reviewed changes

artursapek closed this Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDK: Add core/base files and unit tests #3504

SDK: Add core/base files and unit tests #3504

barnjamin commented Nov 7, 2023

SEJeff left a comment

SEJeff Dec 12, 2023

barnjamin Dec 13, 2023

artursapek Jan 4, 2024

SEJeff Dec 12, 2023

artursapek Jan 4, 2024

barnjamin Jan 4, 2024

nonergodic Jan 4, 2024 •

edited

Loading

artursapek Jan 4, 2024

barnjamin Jan 4, 2024

artursapek commented Jan 4, 2024

kev1n-peters commented Jan 4, 2024 •

edited

Loading

nonergodic commented Jan 4, 2024 •

edited

Loading

nonergodic commented Jan 4, 2024 •

edited

Loading

barnjamin commented Jan 4, 2024 •

edited

Loading

nonergodic commented Jan 4, 2024

kev1n-peters commented Jan 4, 2024

artursapek commented Jan 4, 2024

nonergodic commented Jan 4, 2024

artursapek commented Jan 5, 2024 •

edited

Loading

artursapek Jan 5, 2024

barnjamin Jan 5, 2024

artursapek left a comment

		RUN apt-get update && apt-get -y install \
		git python3 make curl netcat

-RUN apt-get update && apt-get -y install \
-  git python3 make curl netcat
+RUN apt-get update && apt-get -y install --no-install-recommends \
+  git python3 make curl netcat && \
+  apt-get clean && \
+  rm -rf /var/lib/apt/lists/*

		@@ -0,0 +1,59 @@
		export type Extends<T, U> = [T] extends [U] ? true : false;

		export type Function<P extends any[] = any[], R = any> = (...args: P) => R;

SDK: Add core/base files and unit tests #3504

SDK: Add core/base files and unit tests #3504

Conversation

barnjamin commented Nov 7, 2023

SEJeff 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

nonergodic Jan 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

artursapek commented Jan 4, 2024

kev1n-peters commented Jan 4, 2024 • edited Loading

nonergodic commented Jan 4, 2024 • edited Loading

nonergodic commented Jan 4, 2024 • edited Loading

barnjamin commented Jan 4, 2024 • edited Loading

nonergodic commented Jan 4, 2024

kev1n-peters commented Jan 4, 2024

artursapek commented Jan 4, 2024

nonergodic commented Jan 4, 2024

artursapek commented Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artursapek left a comment

Choose a reason for hiding this comment

nonergodic Jan 4, 2024 •

edited

Loading

kev1n-peters commented Jan 4, 2024 •

edited

Loading

nonergodic commented Jan 4, 2024 •

edited

Loading

nonergodic commented Jan 4, 2024 •

edited

Loading

barnjamin commented Jan 4, 2024 •

edited

Loading

artursapek commented Jan 5, 2024 •

edited

Loading