Contracts top level #422

cam-schultz · 2024-07-11T21:42:29Z

Why this should be merged

Converts the repo to be a top-level forge project. The new repo structure is as follows:

teleporter/
    lib/
    contracts/
        Teleporter/
        Utilities/
        Mocks/
    foundry.toml
    remappings.txt

contracts-monorepo is the staging branch for all changes related to #419

How this works

Reogranizes the repo into the above structure

How this was tested

CI

How is this documented

Updated paths in READMEs

geoff-vball

Bindings checker has something failing. Otherwise LGTM

foundry.toml

michaelkaplan13 · 2024-07-12T18:08:30Z

.gitmodules

+	branch = v1
+	path = lib/forge-std
+	url = https://github.com/foundry-rs/forge-std
+	ignore = dirty
 [submodule "lib/openzeppelin-contracts"]
 	branch = v4.8.1
-[submodule "contracts/lib/subnet-evm"]
-	path = contracts/lib/subnet-evm
+	path = lib/openzeppelin-contracts
+	url = https://github.com/openzeppelin/openzeppelin-contracts
+	ignore = dirty


Did you mean to change the lib/forge-std branch and/or add the ignore = dirty flags here? Seems like just changing the paths would be sufficient.

When putting together this change, I noticed a few things:

our forge-std submodule was using a different branch than openzeppelin-contract's submodule

We weren't specifying a path to our forge-std submodule in remappings.txt

From what I could tell, this means that the exact forge-std version that's used is whatever one is found first when resolving include paths. To get around this, I added forge-std to remappings.txt explicitly, and updated our submodule to use the same version as openzeppelin-contracts

As for ignore = dirty, that is added as a developer convenience. I noticed that when playing with submodules it was quite easy to get to a state in which no submodule changes were made, but it was still showing as modified in git status. ignore = dirty shouldn't impact CI or fresh clones.

The base branch was changed.

.gitmodules

minghinmatthewlam · 2024-07-31T19:29:23Z

contracts/governance/tests/ValidatorSetSigTests.t.sol

@@ -5,7 +5,7 @@

 pragma solidity 0.8.25;

-import {Test} from "forge-std/Test.sol";
+import {Test} from "@forge-std/Test.sol";


did we need this change? I think this was working previously from the auto remappings, but we could consider turning auto remappings off to be more explicit

nvm I see that it's updated in remappings. If we're going to explicitly set it for forge-std that differs from the default, might want to consider turning off auto remapping detection

I was encountering a scenario where the auto remapping used open-zeppelin-contracts' forge-std instead of ours. IMO we should be explicit wherever possible to prevent this sort of ambiguity.

i'm in favor of copying the remappings we need, then disabling auto remapping detection

minghinmatthewlam

mostly LGTM, left some comments

minghinmatthewlam · 2024-07-31T19:30:19Z

contracts/teleporter/registry/tests/GetTeleporterMessengerTests.t.sol

@@ -10,14 +10,6 @@ import {ITeleporterMessenger} from "@teleporter/ITeleporterMessenger.sol";
 import {TeleporterMessenger} from "@teleporter/TeleporterMessenger.sol";

 abstract contract GetTeleporterMessengerTest is BaseTeleporterRegistryAppTest {
-    function testGetTeleporterMessengerBasic() public {


was there a reason to move this

seems like a lint with the new version, did it hit a lint rule?

Yeah, the linter complained about public view functions being before public ones. solc was throwing warnings saying that this function could be marked view. I'm not sure what change prompted solc to start warning though.

minghinmatthewlam · 2024-07-31T19:33:47Z

contracts/governance/tests/ValidatorSetSigTests.t.sol

@@ -5,7 +5,7 @@

 pragma solidity 0.8.25;

-import {Test} from "forge-std/Test.sol";
+import {Test} from "@forge-std/Test.sol";


nvm I see that it's updated in remappings. If we're going to explicitly set it for forge-std that differs from the default, might want to consider turning off auto remapping detection

minghinmatthewlam · 2024-07-31T19:37:15Z

do we need to update .gitignore? For example I see contracts/doc but dk if we still use that at all.

cam-schultz · 2024-07-31T19:39:38Z

do we need to update .gitignore? For example I see contracts/doc but dk if we still use that at all.

Let's continue ignoring generated docs from forge docs. I've updated the path to reflect the updated repo structure.

geoff-vball

👍

minghinmatthewlam

lgtm 🙌

michaelkaplan13

🚢

meaghanfitzgerald · 2024-08-01T16:21:15Z

Next time the file structures of any of the .mds change, pls ping @laviniat1996. These files are referenced by a plugin in the docs and when the path changes it causes the build of https://github.com/ava-labs/avalanche-docs to break.

cam-schultz added 7 commits July 11, 2024 10:22

libs to top level

ab29ee4

remove old libs

ba00a78

subnet-evm v0.6.2

8612bb8

update remappings

5591ed8

top level foundry project

27feb51

update forget-std ver

a1c1330

remove contracts/src

821c980

cam-schultz requested a review from a team as a code owner July 11, 2024 21:42

cam-schultz requested review from iansuvak, feuGeneA, geoff-vball, minghinmatthewlam, bernard-avalabs and michaelkaplan13 July 11, 2024 21:42

cam-schultz added 9 commits July 11, 2024 16:45

update bindings

6a22956

update paths in jobs

ca9155a

format

a6a4da9

update path

ddcf620

subnet-evm v0.6.1

deb2569

forge-std v1

bdd9297

bump foundry version

4bc2441

bump foundry ver

78212b2

format

2273773

geoff-vball reviewed Jul 12, 2024

View reviewed changes

update bindings

6000426

iansuvak previously approved these changes Jul 12, 2024

View reviewed changes

ignore test compiler warnings

d6fd2cc

cam-schultz requested a review from geoff-vball July 12, 2024 14:41

cam-schultz commented Jul 12, 2024

View reviewed changes

foundry.toml Outdated Show resolved Hide resolved

michaelkaplan13 reviewed Jul 12, 2024

View reviewed changes

cam-schultz requested review from michaelkaplan13 and geoff-vball July 17, 2024 17:15

cam-schultz changed the base branch from contracts-monorepo to main July 17, 2024 17:15

cam-schultz added the DO NOT MERGE This PR is not meant to be merged in its current state label Jul 17, 2024

cam-schultz mentioned this pull request Jul 23, 2024

Upgradeability for TeleporterUpgradeable and TeleporterOwnerUpgradeable #397

Merged

cam-schultz added 2 commits July 26, 2024 11:26

Partial Merge branch 'main' into contracts-top-level

fc4120c

finish merge

93fe8b5

cam-schultz requested a review from iansuvak July 26, 2024 17:19

Merge branch 'main' into contracts-top-level

56c416e

minghinmatthewlam reviewed Jul 31, 2024

View reviewed changes

.gitmodules Outdated Show resolved Hide resolved

remove ignore=dirty

fd5b9f6

cam-schultz removed the DO NOT MERGE This PR is not meant to be merged in its current state label Jul 31, 2024

minghinmatthewlam reviewed Jul 31, 2024

View reviewed changes

update docs path

fb216a7

geoff-vball previously approved these changes Jul 31, 2024

View reviewed changes

disable auto remapping

fae3473

cam-schultz dismissed geoff-vball’s stale review via fae3473 July 31, 2024 19:51

cam-schultz requested review from minghinmatthewlam and geoff-vball July 31, 2024 19:51

minghinmatthewlam approved these changes Jul 31, 2024

View reviewed changes

geoff-vball approved these changes Jul 31, 2024

View reviewed changes

michaelkaplan13 approved these changes Jul 31, 2024

View reviewed changes

cam-schultz merged commit e9b5f51 into main Jul 31, 2024
14 checks passed

cam-schultz deleted the contracts-top-level branch July 31, 2024 20:09

meaghanfitzgerald mentioned this pull request Aug 1, 2024

fix sourceBaseUrl paths after teleporter file renaming ava-labs/avalanche-docs#1808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contracts top level #422

Contracts top level #422

cam-schultz commented Jul 11, 2024

geoff-vball left a comment

michaelkaplan13 Jul 12, 2024

cam-schultz Jul 17, 2024

minghinmatthewlam Jul 31, 2024

minghinmatthewlam Jul 31, 2024

cam-schultz Jul 31, 2024

minghinmatthewlam Jul 31, 2024

minghinmatthewlam left a comment

minghinmatthewlam Jul 31, 2024

minghinmatthewlam Jul 31, 2024

cam-schultz Jul 31, 2024

minghinmatthewlam Jul 31, 2024

minghinmatthewlam commented Jul 31, 2024

cam-schultz commented Jul 31, 2024

geoff-vball left a comment

minghinmatthewlam left a comment

michaelkaplan13 left a comment

meaghanfitzgerald commented Aug 1, 2024

Contracts top level #422

Contracts top level #422

Conversation

cam-schultz commented Jul 11, 2024

Why this should be merged

How this works

How this was tested

How is this documented

geoff-vball 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

minghinmatthewlam 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

minghinmatthewlam commented Jul 31, 2024

cam-schultz commented Jul 31, 2024

geoff-vball left a comment

Choose a reason for hiding this comment

minghinmatthewlam left a comment

Choose a reason for hiding this comment

michaelkaplan13 left a comment

Choose a reason for hiding this comment

meaghanfitzgerald commented Aug 1, 2024