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

Implement MIP-58 - Trusted Relayer #903

Merged
merged 78 commits into from
Jan 14, 2025
Merged

Conversation

0xmovses
Copy link
Contributor

@0xmovses 0xmovses commented Nov 27, 2024

Summary

The feature PR contains all the changes in order to implement the Trusted Relayer as specified in MIP-58

Changelog

  • Removes old solidity bridge contracts
  • Removes old implementation on relayer for HTLC protocol (RFC-40)
  • Updates to ETH and Movement Clients
  • Updates to the State Machine Relayer Logic
  • New smart contract NativeBridge.sol (this handles both L1 -> L2 and L2 ->L1 directions on ETH in a single contract)
  • New Solidity Unit Testing
  • New integration testing for client
  • New e2e test
  • New setup logic for integration testing against the Full Node (no longer using movement CLI to spin up the network)

Testing

NB. For tests we prefer to run them by file when running the entire test binary in one command, sometimes a client test fails (flaky). A fix for this is simply to force restart the anvil node (tested this fix locally and it works, will arrive shortly in a follow up PR).

  1. Remove old state: rm -rf .data/ && rm -rf .movement/
  2. Run the full node (Await for process to fully complete, make a ☕ ):
CELESTIA_LOG_LEVEL=FATAL CARGO_PROFILE=release CARGO_PROFILE_FLAGS=--release nix develop --extra-experimental-features nix-command --extra-experimental-features flakes --command bash -c "just bridge native build.setup.eth-local.celestia-local.bridge --keep-tui"
  1. Run all e2e tests (checks both directions):
DOT_MOVEMENT_PATH="$(pwd)/.movement" cargo test --test bridge_e2e_test_framework -- --nocapture --test-threads=1
  1. Run the eth client integration tests
DOT_MOVEMENT_PATH="$(pwd)/.movement" cargo test --test client_eth_tests -- --nocapture --test-threads=1
  1. Run the mvt client integration tests
DOT_MOVEMENT_PATH="$(pwd)/.movement" cargo test --test client_mvt_tests -- --nocapture --test-threads=1
  1. And the rest ..
DOT_MOVEMENT_PATH="$(pwd)/.movement" cargo test --test bridge_rest -- --nocapture --test-threads=1
DOT_MOVEMENT_PATH="$(pwd)/.movement" cargo test --test bridge_rest -- --nocapture --test-threads=1

Outstanding issues

See two outstanding cases yet to be fixed in the tracking issue #914. These are ignored currently so as not to block deployment and Front End testing, they can be fixed in paralell with this work.

Icarus131 and others added 29 commits November 9, 2024 16:47
…ts-missing

Exploratory Branch for Addressing Missing Bridge Indexer Events
…ontracts

Native Bridge Solidity Contracts
@0xmovses 0xmovses requested a review from andygolay as a code owner November 27, 2024 15:25
@andygolay
Copy link
Contributor

andygolay commented Dec 16, 2024

checks-all.yml is throwing this error:

[Invalid workflow file: .github/workflows/checks-all.yml#L349](https://github.com/movementlabsxyz/movement/actions/runs/12354119083/workflow)
The workflow is not valid. .github/workflows/checks-all.yml (Line: 349, Col: 1): Unexpected value 'bridge-client-integration'

I saw your note @0xmovses, that you're working on CI in a different PR so I'll leave it as is in this branch since you're already on it.

Copy link
Contributor

@andygolay andygolay left a comment

Choose a reason for hiding this comment

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

E2E and client integration tests pass (other than sometimes flaky behavior when running tests together in one command... generally they pass reliably though).

Approving, pending resolution of Liam's change requests.

@0xmovses
Copy link
Contributor Author

@andygolay we should ignore the cicd:bridge label for now as its not stable yet. Tests to be ran by reviewers locally for now.

@0xmovses 0xmovses requested a review from l-monninger December 17, 2024 09:56
vm.warp(1 days - 1);

uint256 snapshot = vm.snapshot();

if (perTransfer >= insuranceBalance / 4) {
// if the sum of the two transfers is below the rate limit, the second transfer should go through, else it fails
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would still do this in a loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Primata is out sick so I'll add this, thank you.

}
BridgeContractEvent::Completed(_detail) => {
// Unwrap tested before in validate_state() state can be unwrap
let state = state_opt.unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@musitdev @0xmovses I still don't think this is the right approach. It's quite easy to map and error here.

@0xmovses
Copy link
Contributor Author

Let's hold on merge until Liam's recent remarks are addressed.

@0xmovses
Copy link
Contributor Author

0xmovses commented Dec 19, 2024

@l-monninger see the issue with links to your review comments: #971

There are unverified commits in the PR so will need you to do the Squash and merge.

@0xmovses
Copy link
Contributor Author

All checks passed.

@l-monninger l-monninger merged commit 05d31c4 into main Jan 14, 2025
276 of 277 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants