Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove awm-relayer dependency #52

Merged
merged 19 commits into from
Oct 18, 2023
Merged

Remove awm-relayer dependency #52

merged 19 commits into from
Oct 18, 2023

Conversation

gwen917
Copy link
Contributor

@gwen917 gwen917 commented Oct 6, 2023

Why this should be merged

  • Remove awm-relayer dependency in teleporter repo
  • Use the ABI generated from abigen.

How this works

  • The Teleporter packing/unpacking and utility functions are moved from awm-relayer to teleporter. This PR updates the usage in awm-relayer: Remove Teleporter interfacing code icm-services#61
  • The ABI bindings and utilities are also organized into the following directory structure:
teleporter/
 |--- go-utils/
 |        |--- abi-bindings/
 |        |        |--- Teleporter/
 |        |        |         |--- TeleporterMessenger/
 |        |        |         |        |---TeleporterMessenger.go
 |        |        |         |        |--- packing.go
 |        |--- utils/
 |        |        |--- deployment_utils.go
 |        |        |--- gas_utils.go

Bindings for future languages can be added in the future under a similar directory structure.

How this was tested

  • CI: unit tests, end to end tests

@gwen917 gwen917 changed the title remove awm-relayer dependency Remove awm-relayer dependency Oct 6, 2023
Copy link
Contributor

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

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

Let's go with the following directory structure:

teleporter/
 |--- go/
 |        |--- abi-bindings/
 |        |        |--- Teleporter/
 |        |        |         |--- TeleporterMessenger/
 |        |        |         |        |---TeleporterMessenger.go
 |        |        |         |        |--- packing.go
 |        |--- teleporter-utilities/
 |        |        |--- utils.go

We can put the packing/unpacking functions in packing.go and extra utilities in utils.go

tests/utils/abis.go Outdated Show resolved Hide resolved
tests/utils/utils.go Outdated Show resolved Hide resolved
tests/utils/utils.go Outdated Show resolved Hide resolved
tests/utils/utils.go Outdated Show resolved Hide resolved
tests/utils/network_setup.go Outdated Show resolved Hide resolved
tests/utils/abis.go Outdated Show resolved Hide resolved
Copy link
Contributor

@geoff-vball geoff-vball left a comment

Choose a reason for hiding this comment

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

Just a few small comments. Thanks for doing this!

tests/utils/abis.go Outdated Show resolved Hide resolved
tests/utils/network_setup.go Outdated Show resolved Hide resolved
tests/utils/utils.go Outdated Show resolved Hide resolved
@gwen917 gwen917 marked this pull request as ready for review October 9, 2023 17:07
bernard-avalabs
bernard-avalabs previously approved these changes Oct 9, 2023
Copy link
Contributor

@bernard-avalabs bernard-avalabs left a comment

Choose a reason for hiding this comment

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

LGTM

go/abi-bindings/Teleporter/TeleporterMessenger/packing.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

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

All the changes LGTM.

Can you add a README under go/abi-bindings describing the directory contents? Specifically, note that the folders/files with the same names as the Solidity contracts are created by the abi_go_bindings.sh script, and also note that the packing.go files are written manually on a case-by-case basis, since abigen does not generate them out of the box.

go/teleporter-utilities/utils.go Outdated Show resolved Hide resolved
@michaelkaplan13
Copy link
Collaborator

Looks like there are still a few open comments from @cam-schultz on this PR.

The only additional one I have is that it's a little odd as-is because contract-deployment/ contains golang files that are not under the go/ directory. I'm open to either moving it under go/, or removing the go/ directory all together and moving abi-bindings/ back to the top level. Curious if anyone has opinions here?

cc @bernard-avalabs @geoff-vball @minghinmatthewlam

@cam-schultz
Copy link
Contributor

The only additional one I have is that it's a little odd as-is because contract-deployment/ contains golang files that are not under the go/ directory. I'm open to either moving it under go/, or removing the go/ directory all together and moving abi-bindings/ back to the top level. Curious if anyone has opinions here?

My thought was to structure the directories in such a way that would allow bindings to other languages, not just Go. However, a better layout would be putting go beneath abi-bindings, like so: abi-bindings/go-bindings. Other language bindings would be side-by-side with go-bindings. If we go this route, I definitely don't think the deployment utils should be in go-bindings.

It seems reasonable that we may want to support other language bindings for Teleporter in the future, especially as Teleporter development expands beyond in-house. I'd advocate for having the directory structure we go with now reflect that.

@minghinmatthewlam
Copy link

The only additional one I have is that it's a little odd as-is because contract-deployment/ contains golang files that are not under the go/ directory. I'm open to either moving it under go/, or removing the go/ directory all together and moving abi-bindings/ back to the top level. Curious if anyone has opinions here?

My thought was to structure the directories in such a way that would allow bindings to other languages, not just Go. However, a better layout would be putting go beneath abi-bindings, like so: abi-bindings/go-bindings. Other language bindings would be side-by-side with go-bindings. If we go this route, I definitely don't think the deployment utils should be in go-bindings.

It seems reasonable that we may want to support other language bindings for Teleporter in the future, especially as Teleporter development expands beyond in-house. I'd advocate for having the directory structure we go with now reflect that.

It seems strange to me to move the abi bindings under a go directory when we have the go.mod and go.sum in the root of the repo, and the e2e tests/contract deployments are also in go. I feel part of the repo is factoring in other languages in the future, while other parts only support go. Would consider removing the separation of go directories for now, and defer refactoring to when we do add other language support.

@michaelkaplan13
Copy link
Collaborator

It seems strange to me to move the abi bindings under a go directory when we have the go.mod and go.sum in the root of the repo, and the e2e tests/contract deployments are also in go. I feel part of the repo is factoring in other languages in the future, while other parts only support go. Would consider removing the separation of go directories for now, and defer refactoring to when we do add other language support.

I tend to agree now that you call that out. Cam noted that the go.mod for blst is in the root directory of their repository even when all of the golang code is in a subdirectory, but the e2e tests and contract deployment parts are definitely a little odd where they live currently. I'd be on board with either moving both of them under go/ (removing the "-utils"), or getting rid of go-utils/ all together for now and deferring the refactoring for other adding other languages to when that time comes.

@cam-schultz
Copy link
Contributor

I tend to agree now that you call that out. Cam noted that the go.mod for blst is in the root directory of their repository even when all of the golang code is in a subdirectory, but the e2e tests and contract deployment parts are definitely a little odd where they live currently. I'd be on board with either moving both of them under go/ (removing the "-utils"), or getting rid of go-utils/ all together for now and deferring the refactoring for other adding other languages to when that time comes.

I'd strongly prefer to decide on a directory structure that takes into account other language bindings now, rather than introduce breaking changes to downstream dependencies when we decide to add additional language bindings. How about we move the contract deployment app and the test suites+utilities under go/, and put the bindings under abi-bindings/go? That way abi-bindings can be easily expanded to include other languages in the future. I think this directory structure would only be confusing with a single language binding, but if abi-bindings had multiple subdirectories, one of which is go/, it would be clear what that directory represents.

@michaelkaplan13
Copy link
Collaborator

michaelkaplan13 commented Oct 18, 2023

All the changes LGTM, would you also mind adding this README that you mentioned prior?

Can you add a README under go/abi-bindings describing the directory contents? Specifically, note that the folders/files with the same names as the Solidity contracts are created by the abi_go_bindings.sh script, and also note that the packing.go files are written manually on a case-by-case basis, since abigen does not generate them out of the box.

Copy link
Collaborator

@michaelkaplan13 michaelkaplan13 left a comment

Choose a reason for hiding this comment

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

LGTM!

with:
submodules: recursive

- name: Run ABI Binding Unit Tests

Choose a reason for hiding this comment

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

nit: these are not just abi binding unit tests right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Added a cd abi-bindings/go so it will only run the ABI bindings unit tests. Running go test ./... will call the E2E tests. They won't run since the necessary env var isn't set, but that's still not what we're targeting here.

Copy link

@minghinmatthewlam minghinmatthewlam left a comment

Choose a reason for hiding this comment

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

LGTM, left some nits

Copy link

@minghinmatthewlam minghinmatthewlam left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants