Update README with dependency notes #456
Conversation
iansuvak
requested review from
feuGeneA,
geoff-vball,
minghinmatthewlam,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
| > [!NOTE] | ||
| > We require any commits referenced in our `main` branch to be present in the default branches of the repositories above, but during active development it might be useful to work against changes in progress that are still on feature branches. | ||
|
|
||
| When developing such features that require updates to one or more of the above, care must be taken to understand where the relevant code comes from. The binaries of applications built in this repo are built against versions referenced in the `go.mod` file. The E2E tests run against a simulated network running locally that is started by calling a separately compiled `avalanchego` binary as well as its plugins. These are compiled based on the values of `AVALANCHEGO_VERSION` in the local checkout of `subnet-evm` when running the tests locally and directly in this repository's `./scripts/versions.sh` when running E2E tests remotely through GitHub actions. |
There was a problem hiding this comment.
This is currently only in a branch, but we equate the versions in go.mod and the binary installed for the e2e tests like so in teleporter: https://github.com/ava-labs/teleporter/blob/staking-contract/scripts/versions.sh#L32
We could simplify this documentation by incorporating this approach here.
There was a problem hiding this comment.
Yeah but it's possible and valid to do both. At the very least this explains which code comes from where.
There was a problem hiding this comment.
I.e. it might be useful to test a version of relayer code against a tmpnet running a separate version of the code. I agree that we should make the extract-commit change in this repo as well, but for development and troubleshooting I still want some version of the documentation above.
README.md
Outdated
|
|
||
| `avalanchego` and `coreth` have a direct circular dependency and this repository is only indirectly dependent on `coreth` but directly on `avalanchego`. Therefore if any updates are required from the `coreth` side, a corresponding `avalanchego` commit referencing those changes is required. On the other hand `subnet-evm` just depends directly on `avalanchego`. | ||
|
|
||
| The most complicated example case that can arise above is that a feature depends on a new change in `coreth`. In this case, the developer needs to: |
There was a problem hiding this comment.
Looks like this section is incomplete.
There was a problem hiding this comment.
The cases below this actually explain the workflow that I'm introducing here. I can make them bold instead of headings to make it clearer perhaps?
There was a problem hiding this comment.
I think it's just the wording, usually when using : i expect a list to follow right after
There was a problem hiding this comment.
"in this case the developer would need to perform the following steps for either remote or local e2e testing cases." or similar
There was a problem hiding this comment.
Yeah, the separation between headings is confusing. Perhaps explicitly mention the headings, or mention the "following sections"
README.md
Outdated
| Local testing through running `./scripts/e2e_test.sh` also requires local checkout of the `subnet-evm` repository with the commit matching the `go.mod` and `SUBNET_EVM_VERSION`. | ||
| If they do not match, tests might still run but fail with unexpected errors. |
There was a problem hiding this comment.
Not necessary to merge this, but we should look into removing the local subnet-evm requirement for awm-relayer, to match teleporter
There was a problem hiding this comment.
Yeah I considered this originally but I do find this useful to be able to work against purely local versions of subnet-evm without needing to pollute the remote with branches for local testing. We could make it optional and if not present pull subnet-evm based on go.mod/versions.sh
There was a problem hiding this comment.
That would be useful, though we should be consistent between repos. We should also support that more generically for all upstreams, including avalanchego and coreth.
| 3. [subnet-evm](https://github.com/ava-labs/subnet-evm) | ||
|
|
||
| > [!NOTE] | ||
| > We require any commits referenced in our `main` branch to be present in the default branches of the repositories above, but during active development it might be useful to work against changes in progress that are still on feature branches. |
There was a problem hiding this comment.
How do we enforce this?
There was a problem hiding this comment.
Code review for now? I assume we could write a github action that does it as well but doesn't seem like high priority.
There was a problem hiding this comment.
if this was not done correctly, would the e2e tests build correctly?
There was a problem hiding this comment.
Yeah they would. E2E tests just need the commits to be reachable on the remote. They don't care if it's in a feature or default branch
Co-authored-by: cam-schultz <[email protected]> Signed-off-by: Ian Suvak <[email protected]>
Co-authored-by: bernard-avalabs <[email protected]> Signed-off-by: Ian Suvak <[email protected]>
Co-authored-by: bernard-avalabs <[email protected]> Signed-off-by: Ian Suvak <[email protected]>
Co-authored-by: bernard-avalabs <[email protected]> Signed-off-by: Ian Suvak <[email protected]>
Co-authored-by: bernard-avalabs <[email protected]> Signed-off-by: Ian Suvak <[email protected]>
Why this should be merged
Addresses #428
Not sure if this is the best place for it but top level
README.mdmade more sense to me thanCONTRIBUTING.mdas it's too specific to this case instead of general contribution to the repo.How this works
N/A
How this was tested
N/A
How is this documented
N/A