relayer: use @beamer-bridge/deployments package by compojoom · Pull Request #1804 · beamer-bridge/beamer

compojoom · 2023-05-11T11:32:11Z

The abis and contract addresses are now taken out of the @beamer-bridge/deployments package.

GabrielBuragev

Looks good. Happy to see we are eliminating the static contract address strings!

relayer/package.json

GabrielBuragev · 2023-05-16T08:54:22Z

Can we also address this https://github.com/beamer-bridge/beamer/pull/1804/files#diff-c3b8b667ddcfc5e232f930bbbc100cced3368cad34183f87aa58d856122841b5R32 ?

This line in the src/services/ethereum.ts file:
// TODO: import from 'deployments' npm package once ready

compojoom · 2023-05-22T07:23:41Z

@GabrielBuragev please re-review. I've changed the PR slightly. So instead of directly using the package where necessary, I kind of re-export the variables from it inside a single deployments file. This should make it easier to update if the structure of the abis/deployments changes again.

Please also pay special attention to the modifications I've done to the ethereum file.

manuelwedler

Just adding two things I noticed because I integrated the package into the frontend

manuelwedler · 2023-05-24T10:01:44Z

relayer/package.json

    "test:unit": "jest unit",
    "test:coverage": "jest --collect-coverage",
-    "generate-types": "typechain --target=ethers-v5 ./src/assets/abi/**.json --out-dir=./types-gen/contracts/",
+    "generate-types": "typechain --target=ethers-v5 ./node_modules/@beamer-bridge/deployments/dist/abis/mainnet/**.json --out-dir=./types-gen/contracts/",


Is it really fine to just use the mainnet abis here? Couldn't there be the case when the goerli and mainnet versions diverge in one package and the relayer should be compatible with both?

I am a little bit worried about this as well. This also means that we will not be able to integrate the relayer in our local e2e tests with the local ABIs.

I am trying to think of ways to avoid this. We could have a prepare or configure script for generating the contract types / ABIs based on a provided --path-to-deployments argument. So, before running the relayer, one would have to run this script (yarn process?) which will generate the types and dump them in the current types-gen/contracts folder.

Same goes for the build process.

With this in place, we could then easily apply your suggestion below:

The ABI can be easily accessed by FillManager__factory.abi. I think it is not necessary to reexport and would also be more consistent with how the contracts are used in the other places.

The reason why i am proposing such a solution with an argument --path-to-deployments is because the e2e tests are storing the deployment artifacts outside of the project directory (temporary folders) which makes it trickier to automate this process by only using the @beamer-bridge/deployments package ..

While trying this approach out, i figured out that we can even do this right now by passing --glob='...pathToAbis' when invoking the yarn generate-types script as this will effectively overwrite the --glob that is already defined in the generate-types yarn command.
This will enable us to specify different set of ABIs for the type generation process. If we follow this approach, then we have to adjust the code to only use the contract types & factories that are auto-generated by TypeChain (basically what you proposed below).
By default, the yarn generate-types will only use the mainnet ABIs. What do you guys think of this?

Tested this out, but unfortunately it only extends the glob patterns instead of overwriting the argument value.

manuelwedler · 2023-05-24T10:08:38Z

relayer/src/common/events/FillInvalidated.ts


 export const parseFillInvalidatedEvent = (logs: Log[]): FillInvalidatedEventData | null => {
-  const iface = new Interface(FillManagerABI);
+  const iface = new Interface(abis.FillManager);


The ABI can be easily accessed by FillManager__factory.abi. I think it is not necessary to reexport and would also be more consistent with how the contracts are used in the other places.

relayer/src/common/events/FillInvalidated.ts

The abis and contract addresses are now taken out of the @beamer-bridge/deployments package. From now on, when we have a new deployment we need to update the package and then create a new build of the relayer.

manuelwedler · 2023-05-31T16:29:57Z

Getting the right ABI versions

@GabrielBuragev and me had a long session about the ABI versions and how to use the right one for each use case.

The issue is that we used to just have one ABI version for the relayer that was changed by copy-paste whenever we needed to. Now we want to use the ABIs from the deployments package and the package provides separate versions for goerli and mainnet.

I would like to split the issue into two separate layers where the ABIs come to effect:

Creating transactions / decoding events with the ABIs
Providing typing using type-chain

Transactions / Events

The relayer is build into one image with the agent. This image is supposed to run on any network a deployment configuration is provided for. This means the relayer needs to be bundled with both ABIs, goerli and mainnet, in order to ensure it works properly.

This can be solved easily by dynamically choosing the correct ABI during runtime. For example like this:

import { abi as FillManagerGoerliABI } from "@beamer-bridge/deployments/dist/abis/goerli/FillManager.json";
import { abi as FillManagerMainnetABI } from "@beamer-bridge/deployments/dist/abis/mainnet/FillManager.json";

export const abis = {
  goerli: {
    FillManager: FillManagerGoerliABI,
  },
  mainnet: {
    FillManager: FillManagerMainnetABI,
  },
};

It might often be the case that we need to provide different ABIs than the ones from the package. For example, we have a new version of the contracts that has not been deployed yet (only locally) and we want to integrate this new version into the relayer.
So in this case, it would be useful to be able to provide a folder of ABIs via a command line argument. This could require some work, because the folder would need to be in a specified format and reading the folder needs to be implemented in the relayer.

Typing

With the typing things get tricky now. There can always only be one version of the ABIs that is used for the typing. And the linting uses the typing, so the CI may fail if the relayer changes in a PR do not fit the ABI used for typing.

A solution that still has its problems is the following: Whenever we change the contracts, we publish the new ABIs in a new folder inside the deployments package. The issue is now we tie the contracts ABI to one version of the deployments, because they are published in the same package. We lose flexibility by this.

Different solutions we had in mind (didn't fully think through all of them):

Always use the mainnet version (as soon as the contract changes and we want to develop against a new version we run into issues)
Splitting the relayer into a goerli and mainnet version (different executables)

compojoom · 2023-06-12T06:18:11Z

Hey guys,

Thanks for researching this and the summary!

I do have some questions as I don't fully understand.

It might often be the case that we need to provide different ABIs than the ones from the package. For example, we have a new version of the contracts that has not been deployed yet (only locally) and we want to integrate this new version into the relayer.
So in this case, it would be useful to be able to provide a folder of ABIs via a command line argument. This could require some work, because the folder would need to be in a specified format and reading the folder needs to be implemented in the relayer.

If we have a different version of the contracts we will have either a dev release or a new major release of the npm package. The relayer needs to be updated to use the new version of the abi package. Why do we need to provide a folder here?

Why is typing a problem if we are updating the package used in the relayer? If we point the relayer to a new version, we adjust the types as needed?

manuelwedler · 2023-06-14T10:37:18Z

If we have a different version of the contracts we will have either a dev release or a new major release of the npm package. The relayer needs to be updated to use the new version of the abi package. Why do we need to provide a folder here?

Why is typing a problem if we are updating the package used in the relayer? If we point the relayer to a new version, we adjust the types as needed?

The npm package has two folders for the ABIs: goerli and mainnet. Where would the version of the ABIs we need be located? Or are both the same? The question of the typing also arises by these two folders. Which one should be used for typing?

At the moment we don't have different releases of the relayer for mainnet / goerli btw.

Also see my questions from which this discussion originated from: #1804 (comment)

istankovic · 2023-08-14T13:21:07Z

@compojoom @GabrielBuragev @manuelwedler what's the state of this?
I see it even has conflicts with current main. 😅

manuelwedler · 2023-08-14T13:46:55Z

We decided the following in our last meeting:
Dynamically decide during runtime which ABI to use for the transactions / events.
For typing, just use Goerli ABI.

Due to priorities this wasn't finished yet.

compojoom requested a review from GabrielBuragev May 11, 2023 11:32

compojoom force-pushed the relayer/adjust_abis_and_addresses branch from 2817558 to bd8f266 Compare May 11, 2023 11:39

GabrielBuragev approved these changes May 15, 2023

View reviewed changes

relayer/package.json Outdated Show resolved Hide resolved

compojoom force-pushed the relayer/adjust_abis_and_addresses branch from b6ac66c to ecae5a4 Compare May 22, 2023 07:18

compojoom requested a review from GabrielBuragev May 22, 2023 07:20

manuelwedler reviewed May 24, 2023

View reviewed changes

GabrielBuragev reviewed May 24, 2023

View reviewed changes

relayer/src/common/events/FillInvalidated.ts Outdated Show resolved Hide resolved

GabrielBuragev force-pushed the relayer/adjust_abis_and_addresses branch from ecae5a4 to 2188936 Compare May 26, 2023 15:46

compojoom and others added 2 commits May 31, 2023 11:27

relayer: use @beamer-bridge/deployments package

36b54bb

The abis and contract addresses are now taken out of the @beamer-bridge/deployments package. From now on, when we have a new deployment we need to update the package and then create a new build of the relayer.

relayer: use auto-generated types as interfaces

8cddef7

GabrielBuragev force-pushed the relayer/adjust_abis_and_addresses branch from b459360 to 8cddef7 Compare May 31, 2023 10:05

GabrielBuragev mentioned this pull request Jun 20, 2023

relayer: use resolver block number per chain #1961

Merged

istankovic force-pushed the main branch 4 times, most recently from 93aa1c1 to 1ec78d7 Compare July 31, 2023 21:43

Conversation

compojoom commented May 11, 2023

Uh oh!

GabrielBuragev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GabrielBuragev commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compojoom commented May 22, 2023

Uh oh!

manuelwedler left a comment

Choose a reason for hiding this comment

Uh oh!

manuelwedler May 24, 2023

Choose a reason for hiding this comment

Uh oh!

GabrielBuragev May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabrielBuragev May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabrielBuragev May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabrielBuragev May 31, 2023

Choose a reason for hiding this comment

Uh oh!

manuelwedler May 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

manuelwedler commented May 31, 2023

Getting the right ABI versions

Transactions / Events

Typing

Uh oh!

compojoom commented Jun 12, 2023

Uh oh!

manuelwedler commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istankovic commented Aug 14, 2023

Uh oh!

manuelwedler commented Aug 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GabrielBuragev commented May 16, 2023 •

edited

Loading

GabrielBuragev May 30, 2023 •

edited

Loading

GabrielBuragev May 30, 2023 •

edited

Loading

GabrielBuragev May 30, 2023 •

edited

Loading

manuelwedler commented Jun 14, 2023 •

edited

Loading