feat: implements BlockNumberService #13

0xyaco · 2024-07-26T19:42:17Z

🤖 Linear

Closes GRT-38
Part of GRT-77

Description

Refactors the CAIP2 chain id class, using it as a static module that only validates a string being compliant.
Implements BlockNumberService, enabling the agent to get from multiple chains their block numbers at a particular timestamp.

linear · 2024-07-26T19:42:19Z

Follow Figma diagrams: https://www.figma.com/board/BbciqJb5spg35ZglTsRRBb/Offchain?node-id=233-2491&t=fNurQJG1q77rduKS-4to implement BlocknumberService.

AC:

BlocknumberService successfully implemented
CAIP-2 standard is used for blockchains ids (https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-2.md)
BlocknumberService should have the capability to distinguish between an EVM or nonEVM chain to use the corresponding provider to fetch blocknumbers in the future (EVMProvider, BlockmetaProvider)
Unit tests

0xyaco · 2024-07-26T19:47:11Z

packages/blocknumber/src/utils/caip/caip2.ts

+export const chains = {
+    mainnet: "eip155:1",
+    polygon: "eip155:137",
+    arbitrum: "eip155:42161",
+} as Record<SupportedChains, Caip2ChainId>;


This is something that feels weird for me. It works, but feels super weird. Any suggestions?

Supported chains should be part of the config file from the runtime or on a shared module, not here i think , this is just an abstraction of the CAIP2

0xyaco · 2024-07-26T19:50:35Z

packages/blocknumber/src/services/blockNumberService.ts

+        switch (chainId) {
+            case "eip155:1":
+                return new EvmBlockNumberProvider(client, {
+                    blocksLookback: 10_000n,
+                    deltaMultiplier: 2n,
+                });
+
+            default:
+                throw new UnsupportedChain(chainId);
+        }


@0xkenj1 what do you think about this one? Should we have some kind of config file/constant value with the following attributes per chain?

Provider class to use

blocksLookback

deltaMultiplier

Something like:

providers: - "eip155:1": - provider: EvmBlockNumberProvider blocksLookback: 10000 deltaMultiplier: 2

2 things here:

i would just use the same config for all the evm's for now , to avoid extra complexity. Lets leave some default values, and in the future if we need it we can do an easy refactor to inject the config.

on the switch statement, we can just pick the namespace from the CAIP2 in order to decide which blockNumberService we should instantiate. Lets avoid hardcoding each of the chains.

After reading the code below, the switch statement should run after a validation, like:

if( chainId is not in SupportedChains ){
throw ``;
}

we can inject SupportedChains on the constructor, or having them on the shared module as constants would be good also

Given that we'll be deciding which provider to instantiate based on the namespace, what do you think about building a functionstatic getNamespace(chainId: string | Caip2ChainId) inside Caip2?

And I agree, let's go with a one-size-fits-all config for every chain and later we can add more customization.

Yea, perfect, thats what we need :)

And regarding the supported chains, with this convo I think it'd make more sense to validate the supported chains with a shared module/config file inside the constructor, yeah.

This buildProvider method being a static one and using (at the moment) only the namespace to operate seems like a not so good place. Probably in the future if we define a custom config per chain (for its provider) we'll be able to do something like:

if (config.getProviderConfig(chainId) == null) throw new Error(); switch (namespace) { ... }

0xkenj1 · 2024-07-29T18:45:57Z

packages/blocknumber/src/services/blockNumberService.ts

+        switch (chainId) {
+            case "eip155:1":
+                return new EvmBlockNumberProvider(client, {
+                    blocksLookback: 10_000n,
+                    deltaMultiplier: 2n,
+                });
+
+            default:
+                throw new UnsupportedChain(chainId);
+        }


2 things here:

i would just use the same config for all the evm's for now , to avoid extra complexity. Lets leave some default values, and in the future if we need it we can do an easy refactor to inject the config.

on the switch statement, we can just pick the namespace from the CAIP2 in order to decide which blockNumberService we should instantiate. Lets avoid hardcoding each of the chains.

0xkenj1 · 2024-07-29T18:59:00Z

packages/blocknumber/src/utils/caip/caip2.ts

+export const chains = {
+    mainnet: "eip155:1",
+    polygon: "eip155:137",
+    arbitrum: "eip155:42161",
+} as Record<SupportedChains, Caip2ChainId>;


Supported chains should be part of the config file from the runtime or on a shared module, not here i think , this is just an abstraction of the CAIP2

0xkenj1 · 2024-07-29T19:03:02Z

packages/blocknumber/src/services/blockNumberService.ts

+        switch (chainId) {
+            case "eip155:1":
+                return new EvmBlockNumberProvider(client, {
+                    blocksLookback: 10_000n,
+                    deltaMultiplier: 2n,
+                });
+
+            default:
+                throw new UnsupportedChain(chainId);
+        }


After reading the code below, the switch statement should run after a validation, like:

if( chainId is not in SupportedChains ){
throw ``;
}

we can inject SupportedChains on the constructor, or having them on the shared module as constants would be good also

0xyaco · 2024-07-30T14:56:59Z

Pendingshared modules integration yet

0xkenj1

some findings, but we are almost there :)

GJ ser! 💯

0xkenj1 · 2024-07-31T03:53:17Z

packages/shared/src/constants.ts

+export const supportedChains = {
+    evm: {
+        namespace: "eip155",
+        chains: {


Suggested change

chains: {

reference: {

0xkenj1 · 2024-07-31T03:53:38Z

packages/shared/src/constants.ts

+            ethereum: "eip155:1",
+            polygon: "eip155:137",
+            arbitrum: "eip155:42161",


Suggested change

ethereum: "eip155:1",

polygon: "eip155:137",

arbitrum: "eip155:42161",

ethereum: "1",

polygon: "137",

arbitrum: "42161",

0xkenj1 · 2024-07-31T03:55:33Z

packages/blocknumber/src/utils/caip/caip2.ts

-        this.reference = params.reference;
-    }
-
+export class Caip2 {


Suggested change

export class Caip2 {

export abstract class Caip2 {

Suggested change

export class Caip2 {

export class Caip2Utils {

??

0xkenj1 · 2024-07-31T03:59:48Z

packages/shared/src/constants.ts

@@ -0,0 +1,10 @@
+export const supportedChains = {


i think maybe this should be an env var or configurable by the user insted of part of a shared package.

Each user may want to choose which blockchains they support.

Maybe the input structure should be something like

{ evm: { namespace: "eip155", chains: { ethereum: { reference: "1", rpcs: ["...", "..."] } } }

maybe i would rather pick reference as key of "chains" we are not gona be looking up using human readable, wdyt?

thinking again here. maybe what we need is a user input structure and also another structure of validChains (what we have now) to validate user input against it. make sense ?

You envision something like:

MONITORED_CHAINS=eip155:1,eip155:137 ebo-agent run

(I guess also via a config file apart from env vars)

And that user-input list of chains should be a subset of our supportedChains set of values?

yea, exactly, a subset of the supported chains defined by us. The user should be also able to set rpcUrls for each of the chains.

0xkenj1 · 2024-07-31T04:06:13Z

packages/blocknumber/src/services/blockNumberService.ts

+                return new EvmBlockNumberProvider(
+                    client,
+                    DEFAULT_PROVIDER_CONFIG,
+                    Logger.getInstance(), // Should we drop this arg?


dont think we should, maybe we can use this.logger but its the same at the end 🤣

Aaah yesss let's add a logger member to this service.

0xkenj1 · 2024-07-31T04:08:47Z

packages/blocknumber/src/services/blockNumberService.ts

+        }, [] as string[]);
+    }
+
+    public static buildProvider(


this smells like a Factory method pattern pattern

Whoops missed to answer this one; it is indeed a Factory method pattern! Want me to do some special tweaks on it?

I was wondering if we should remove this method from the BlockNumberService class, i thing it has nothing to do with the main purpose of the class.

Also the static modifier is telling me that this method doesn't belong to this class 🤣

Probably creating a new class called BlocknumberProviderFactory to have an abstraction on the instantiation of the objects

I see. We can extract this to a new class, absolutely, makes testing cleaner too.

0xkenj1 · 2024-07-31T04:15:34Z

packages/blocknumber/src/services/blockNumberService.ts

+    }
+
+    public async getEpochBlockNumbers(timestamp: number, chains: Caip2ChainId[]) {
+        const epochBlockNumbers = await Promise.all(


probably here we will need a ts-retry, to implement a retry policy.

0xkenj1 · 2024-07-31T04:16:39Z

packages/blocknumber/src/services/blockNumberService.ts

+        this.blockNumberProviders = this.buildBlockNumberProviders(chainRpcUrls);
+    }
+
+    public async getEpochBlockNumbers(timestamp: number, chains: Caip2ChainId[]) {


also remember that will be 1 request -> 1 chain

so lets also add a getEpochBlockNumber method

Oh yes.

Do you think leveraging the multiple chains method would be ok? ie:

getEpochBlockNumber(timestamp, chain) { getEpochBlockNumbers(timestamp, [chain]) }

I would do it the other way round, like:

getEpochBlockNumbers(timestamp, [chain]) {
Promise.all([..., getEpochBlockNumbers(timestamp, chain), ...]
}

0xkenj1 · 2024-07-31T04:18:06Z

packages/blocknumber/src/services/blockNumberService.ts

+const DEFAULT_PROVIDER_CONFIG = {
+    blocksLookback: 10_000n,
+    deltaMultiplier: 2n,
+};


shared package? wdyt?

I see where you are going, yes. Probably we could define a type ProviderConfig modeling this config object that goes in the shared package but keep the const here as I suspect these default values will only be used by the blockNumberService internals? Makes sense?

Makes totally sense, i was thinking that maybe it is better having almost all the constants on the centralized shared package, to only update values there when is needed and avoid modifying the class file's.

Lets leave it as it is.

0xyaco · 2024-07-31T19:32:35Z

@0xkenj1 most significant changes are:

Differentiation between the namespace > reference structure for each chain (EBO_SUPPORTED_CHAINS_CONFIG) as suggested
Built the EBO_SUPPORTED_CHAIN_IDS list of CAIP-2 compliant chain ids using EBO_SUPPORTED_CHAINS_CONFIG
Encapsulated the isChainSupported behavior inside BlockNumberService.isChainSupported(chainId) to be ready for whenever we decide to tackle the mechanism to let the user decide which (out of the supported chains defined in these new constants) will be monitoring with the EBO agent.

Let me know what you think!

0xkenj1

GJ!!!!! This is huge!!! 🥇

0xkenj1 · 2024-08-01T16:23:40Z

packages/blocknumber/src/providers/blockNumberProviderFactory.ts

0xnigir1 · 2024-08-01T16:38:15Z

packages/blocknumber/src/providers/blockNumberProviderFactory.ts

+        switch (chainNamespace) {
+            case EBO_SUPPORTED_CHAINS_CONFIG.evm.namespace:
+                return new EvmBlockNumberProvider(client, DEFAULT_PROVIDER_CONFIG, logger);
+
+            default:
+                throw new UnsupportedChain(chainId);
+        }


a switch for just two cases is too much i think but if this is expected to grow then feel free to ignore this and leave it as it is

It will grow, yep

0xnigir1 · 2024-08-01T16:39:16Z

packages/blocknumber/src/services/blockNumberService.ts

missing natspec

Ended up adding docs to the BlockNumberProviderFactory too 👌

linear · 2024-08-28T15:04:15Z

GRT-77 Add constants and types that might be shared across different packages

0xyaco added 23 commits July 18, 2024 18:18

build: separate tsconfig for build stage

06ef8af

feat: set up blocknumber module tests

9802a89

chore: set up blocknumber module linting

546d76e

Merge remote-tracking branch 'origin/dev' into chore/blocknumber-setup

5b57a2d

style: remove total-typescript on non-base tsconfig

f291b45

build: set up build script

0e8fc5a

fix: blocknumber module package config

baa144a

fix: dummy class import module

9965c35

chore: add coverage script and tasks

6215697

chore: remove dummy classes

2748ca2

feat: implement caip-2 compliant chain ids

ab869a4

refactor: create exceptions/ folder

212960a

Merge remote-tracking branch 'origin/dev' into feat/bn-service

21f025b

feat: search block by timestamp with binsearch

ecd91e9

fix: fixed winston dependency version

4a38c03

refactor: rename evmProvider to evmBlockNumberProvider

711216d

refactor: client and search params in evm provider constructor

c7a9035

Merge remote-tracking branch 'origin/dev' into feat/evm-block-search

18d537a

fix: pnpm dependecies mismatch

5ad1369

fix: throw InvalidTimestamp for timestamps prior first block

6875aea

chore: remove dummy classes

80da90d

refactor: move and redefine caip2 chain id

9a60076

feat: implements BlockNumberService

60a57e1

0xyaco changed the title ~~Feat/bn service~~ feat: implements BlockNumberService Jul 26, 2024

0xyaco commented Jul 26, 2024

View reviewed changes

Merge remote-tracking branch 'origin/dev' into feat/bn-service

841dc96

0xyaco force-pushed the feat/bn-service branch from 7a2782e to 841dc96 Compare July 26, 2024 19:53

chore: fix vitest tests location config

cc0a6f0

0xkenj1 requested changes Jul 29, 2024

View reviewed changes

feat: build provider based on chain namespace only

0b44abe

0xyaco added 4 commits July 30, 2024 19:21

Merge remote-tracking branch 'origin/dev' into feat/bn-service

32a49b7

chore: remove old logger class

557a073

fix: remove references from old logger

163669b

refactor: use shared chains

776ba42

0xyaco force-pushed the feat/bn-service branch from 213aae4 to 776ba42 Compare July 31, 2024 00:24

chore: fix turbo config

ea13674

0xyaco marked this pull request as ready for review July 31, 2024 00:40

0xkenj1 requested changes Jul 31, 2024

View reviewed changes

0xyaco added 5 commits July 31, 2024 14:28

refactor: use logger with dependency injection

0c01a4b

refactor: rename Caip2 to Caip2Utils

472dab2

refactor: created BlockNumberProviderFactory

0954b57

feat: add single-chain getEpochBlockNumber

7f0fa3a

refactor: reorganize chains config structure and values

e97d367

0xkenj1 requested review from 0xnigir1 and 0xkenj1 August 1, 2024 16:23

0xkenj1 previously approved these changes Aug 1, 2024

View reviewed changes

packages/blocknumber/src/providers/blockNumberProviderFactory.ts

Copy link

Collaborator

0xkenj1 Aug 1, 2024

Choose a reason for hiding this comment

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

🚀

0xnigir1 reviewed Aug 1, 2024

View reviewed changes

docs: document BlockNumberService and BlockNumberProviderFactory

927d237

0xyaco dismissed 0xkenj1’s stale review via 927d237 August 2, 2024 12:25

0xyaco requested review from 0xnigir1 and 0xkenj1 August 2, 2024 12:28

0xnigir1 approved these changes Aug 2, 2024

View reviewed changes

0xkenj1 approved these changes Aug 2, 2024

View reviewed changes

0xyaco merged commit 995a84a into dev Aug 2, 2024
5 checks passed

0xyaco deleted the feat/bn-service branch August 2, 2024 14:01

feat: implements BlockNumberService #13

feat: implements BlockNumberService #13

Conversation

0xyaco commented Jul 26, 2024 • edited Loading

🤖 Linear

Description

linear bot commented Jul 26, 2024

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

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

0xyaco commented Jul 30, 2024 • edited Loading

0xkenj1 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

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

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

Choose a reason for hiding this comment

0xkenj1 Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

0xyaco commented Jul 31, 2024

0xkenj1 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

linear bot commented Aug 28, 2024

0xyaco commented Jul 26, 2024 •

edited

Loading

0xyaco commented Jul 30, 2024 •

edited

Loading

0xkenj1 Jul 31, 2024 •

edited

Loading