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

Clarify cross-chain applications as examples #194

Merged
merged 22 commits into from
Dec 20, 2023
Merged

Conversation

minghinmatthewlam
Copy link

@minghinmatthewlam minghinmatthewlam commented Dec 12, 2023

Why this should be merged

Fixes #134

How this works

  • Updates to clarify in documentation that cross-chain applications are examples
  • Adds @teleporter in remappings, and used for cross-chain apps
  • Update other READMEs, like adding one for example messenger

How this was tested

CI

How is this documented

READMEs

Copy link

openzeppelin-code bot commented Dec 12, 2023

Clarify cross-chain applications as examples

Generated at commit: a15e7d2fe10fd668e9ef70b56230d1017e63f351

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
0
0
0
5
23
28
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

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.

I see there are some formatting changes here, which are good - but it would be helpful if we had some way to enforce uniform formatting as we do now with our .go files. I think we can use forge fmt, we would just have to add the formatting rules to foundry.toml. We could add forge fmt to abi bindings script (and probably rename it) and then also check it in ci.

contracts/remappings.txt Outdated Show resolved Hide resolved
@michaelkaplan13
Copy link
Collaborator

In each of the example contract .sol files, can we add a comment towards the top stating that it is example code and do not use it in production? Something similar to how Chainlink has it for CCIP examples here.

@minghinmatthewlam
Copy link
Author

I see there are some formatting changes here, which are good - but it would be helpful if we had some way to enforce uniform formatting as we do now with our .go files. I think we can use forge fmt, we would just have to add the formatting rules to foundry.toml. We could add forge fmt to abi bindings script (and probably rename it) and then also check it in ci.

Deferring this to a new issue #198.

geoff-vball
geoff-vball previously approved these changes Dec 13, 2023
geoff-vball
geoff-vball previously approved these changes Dec 14, 2023
contracts/src/CrossChainApplications/README.md Outdated Show resolved Hide resolved
contracts/src/CrossChainApplications/README.md Outdated Show resolved Hide resolved
@@ -2,8 +2,9 @@

This directory includes cross-chain applications that are built on top of the [Teleporter protocol](../Teleporter/README.md).

## Applications
## Example Applications
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add an additional note here that all contracts under /examples are included solely for demonstration purpose and not intended for production use?

Probably also in GETTING_STARTED.md. It will be slight overkill, but I'd definitely prefer to be on the "over communicated point" side here.

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.

Two tiny nits, and I agree with Michael's comment about adding comments wherever we can indicating the contracts are not production worthy.

import {TeleporterRegistry} from "../../../Teleporter/upgrades/TeleporterRegistry.sol";
import {UnitTestMockERC20} from "../../../Mocks/UnitTestMockERC20.sol";
import {TeleporterRegistry} from "@teleporter/upgrades/TeleporterRegistry.sol";
import {UnitTestMockERC20} from "../../../../Mocks/UnitTestMockERC20.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also add a remapping for the Mocks directory?

Co-authored-by: Michael Kaplan <[email protected]>
Signed-off-by: minghinmatthewlam <[email protected]>
cam-schultz
cam-schultz previously approved these changes Dec 19, 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.

LGTM/. One question about vm.stopPrank in the tests.

@minghinmatthewlam minghinmatthewlam merged commit 1a7b124 into main Dec 20, 2023
15 checks passed
@minghinmatthewlam minghinmatthewlam deleted the example-apps branch December 20, 2023 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Clarify CrossChainApplications as examples
4 participants