Clean up how we codegen Config - #60

DZakh · 2024-07-04T12:39:03Z

Fixes comments from Move codegen from Config.res file #59
A step closer to get rid of ChainMap

github-actions · 2024-07-04T12:41:59Z

Here's the diff of the codegen output.🙌🧠🦜

JonoPrest · 2024-07-04T13:52:03Z

+  chainMap: networks->Js.Array2.map(n => {
+    (n.chain, n)
+  })->ChainMap.fromArray->Utils.unwrapResultExn,


This does, kind of, break the reasoning for the ChainMap abstraction.

It's supposed to be that you don't create one without defining all chains. And that way you can always look it up by chain and guarantee that a value exists at that key.

JonoPrest · 2024-07-04T13:54:06Z

-let getConfigByChainId: int => configYaml = chainId =>
-  RegisterHandlers.getChain(
-    Belt.Result.getExn(ChainMap.Chain.fromChainId(chainId)),
-  )->mapChainConfigToConfigYaml
+let getConfigByChainId: int => configYaml = chainId => {
+  let config = Config.getConfig()
+  config.chainMap->ChainMap.get(ChainMap.Chain.fromChainId(chainId)->Belt.Result.getExn)->mapChainConfigToConfigYaml
+}


FYI this was a file that Denham added to make it possible to see values that you defined in your config.yaml from within your handlers. Maybe we can think about if there's better ways during refactor to expose these values to the user.

Yeah, I even found documentation for it. I actually wanted to suggest to replace it with a chainConfig object in a handlers among side event and context.

JonoPrest

@DZakh, can we discuss what you're planning to do with ChainMap?

Otherwise the properties I was looking for with it is:

It should ensure that all chain values are defined.
You should be able to look up by a "chain" key and guarantee the value exists
It should be easy to do immutable changes

If these properties are blocking you or are suboptimal then keen to hear why and what you are thinking.

DZakh · 2024-07-05T09:56:54Z

@JonoPrest A short answer is that I don't see value in It should ensure that all chain values are defined, since it can be replaced with We trust that all chains are codegened when we set config. Also, it's one step closer to having a library-like architecture.

I'll continue doing what I already started in another PR, but let's have a sync on Monday, to ensure that we are on the same page.

JonoPrest · 2024-07-08T09:30:37Z

@JonoPrest A short answer is that I don't see value in It should ensure that all chain values are defined, since it can be replaced with We trust that all chains are codegened when we set config. Also, it's one step closer to having a library-like architecture.

I'll continue doing what I already started in another PR, but let's have a sync on Monday, to ensure that we are on the same page.

The value in the abstraction is that it guarentees lookup by chain. There are a lot of places in the code where we lookup by chain. I'm happy to get rid of unneeded abstractions but this still seems useful to me.

JonoPrest · 2024-07-08T13:20:09Z

+      chain: {
+        id: {{chain_config.network_config.id}},
+      },


Are there other fields that will exist on this chain record?

JonoPrest · 2024-07-08T13:26:20Z

-          chainId->ChainMap.Chain.fromChainId->Utils.unwrapResultExn
+          let chain = chainId->ChainMap.Chain.evm
+          if !(config.chainMap->ChainMap.has(chain)) {
+            Js.Exn.raiseError("No chain with id " ++ chain->ChainMap.Chain.toString ++ " found in config.yaml")


I started creating an error handling "framework" in ErrorHandling.res

It allows you to raise exceptions with logs etc and pass a logger with a context around. I know this is essentially a global function but maybe we can use that pattern or extend on it to make error logs/exceptions more uniform.

JonoPrest · 2024-07-08T13:42:02Z

+let get: (t<'a>, Chain.t) => 'a = (self, chain) =>
+  switch Map.get(self, chain) {
+  | Some(v) => v
+  | None => Js.Exn.raiseError("No chain with id " ++ chain->Chain.toString ++ " found in config.yaml")
+  }


Can we return result<'a> from this and make it a safe API?

JonoPrest · 2024-07-08T13:50:25Z

@DZakh, I think it looks good but lets keep the ChainMap api safe and return result or option type form the get method.

DZakh

It throws, but in the end the safety of using ChainMap should be the same as before:

It used to throw before, now the error message is more readable
There are two ways to create Chain.t
- With ChainMap.Chain.makeUnsafe - used for initial config set up and mocks
- With Config.getChain - which validates that the provided chainId is a part of the defined config. If it's not, it'll throw with a nice error message. Before it returned a result with exception, but we always unwrapped the exception, so the behaviour is not changed
ChainMap.make which allowed to safely create items for all possible cases now replaces with:
- ChainMap.fromArray - it doesn't have any validation but it used only for initial config creation
- ChainMap.map - all the other places use ChainMap.map, so it's guaranteed to have all chains from config

JonoPrest · 2024-07-09T08:42:54Z

 let fromArray: array<(Chain.t, 'a)> => t<'a> = arr => {
-  arr->Map.fromArray(~id=module(Chain.ChainIdCmp))
+  arr->Map.fromArray(~id=module(ChainIdCmp))


Can we call this fromArrayUnsafe or something? This is where it's unsafe IMO

Yeah, I'll do it.

JonoPrest · 2024-07-09T08:47:14Z

+  let makeUnsafe = (~chainId) => {
    id: chainId,
  }
-
-  module ChainIdCmp = Belt.Id.MakeComparableU({
-    type t = t
-    let cmp = (a, b) => Pervasives.compare(a->toChainId, b->toChainId)
-  })
 }


Should we not just make the type directly the id? Until we need more fields?

Makes sense, since it's opaque now

JonoPrest · 2024-07-09T08:50:01Z

    let convertViemDecodedEvent: Viem.decodedEvent<'a> => Viem.decodedEvent<
      Types.{{contract.name.capitalized}}.{{event.name.capitalized}}.eventArgs,
-    > = X.magic
+    > = Utils.magic


I actually regret having a module called Utils 🤣, but it's probably a good place to put this.

Yeah, I thought it's better not to have two Utils modules :)

JonoPrest

Cool, I think its looking good 👍🏼 looks like I'll have some conflicts in the other PRs but will sort that out.

DZakh · 2024-07-09T12:37:48Z

looks like I'll have some conflicts in the other PRs but will sort that out.

Sorry 😔

DZakh added 2 commits July 4, 2024 16:05

Clean up config registering

431eed7

Clean up networks codegen

2682a1c

DZakh requested a review from JonoPrest July 4, 2024 12:39

JonoPrest reviewed Jul 4, 2024

View reviewed changes

DZakh added 3 commits July 8, 2024 14:57

Clean up ChainMap creation

c8844ca

Remove Chain.all

4a032a2

Make ChainMap a non hbs template

5f335d0

JonoPrest reviewed Jul 8, 2024

View reviewed changes

DZakh added 2 commits July 8, 2024 23:04

Safe Chain creation

1f06196

Fix erc20 multichain factory test

6590f85

DZakh commented Jul 8, 2024

View reviewed changes

DZakh requested a review from JonoPrest July 8, 2024 19:28

Move magic to Utils

ca0afc2

JonoPrest reviewed Jul 9, 2024

View reviewed changes

DZakh added 2 commits July 9, 2024 12:53

Clean up unneeded code

e61a5fb

Fixes after review

2ba192e

DZakh requested a review from JonoPrest July 9, 2024 08:56

DZakh added 2 commits July 9, 2024 13:16

Access config from the state

f80d4bb

Rename getConfig to getGenerated

50cc6f8

DZakh force-pushed the dz/chain-map-clean-up branch from f518388 to 50cc6f8 Compare July 9, 2024 09:18

JonoPrest approved these changes Jul 9, 2024

View reviewed changes

DZakh merged commit 6e3de26 into main Jul 9, 2024

Uh oh!

Conversation

DZakh commented Jul 4, 2024

Uh oh!

github-actions Bot commented Jul 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest left a comment

Choose a reason for hiding this comment

Uh oh!

DZakh commented Jul 5, 2024

Uh oh!

JonoPrest commented Jul 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest commented Jul 8, 2024

Uh oh!

DZakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest left a comment

Choose a reason for hiding this comment

Uh oh!

DZakh commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants