movementlabsxyz / movement

The Movement Network is a Move-based L2 on Ethereum.
Apache License 2.0
74 stars 61 forks source link

Feature flag testing bridge code #380

Open 0xmovses opened 2 months ago

0xmovses commented 2 months ago

Summary

We have several methods in the bridge crates that are intended for testing only. For example in MovementClient::new() this actually genning a random address and this should be renamed to MovementClient::new_for_test

This should be feature flagged as such

#[cfg(test)]
pub fn new_for_test() {}

Requirements

0xPrimata commented 2 months ago

Here you mentioned changing new to new_for_test but new_for_test already exists. Not sure how to handle that case. But, ::new is never actually used. In the meantime, EthClient::new is being used, new_for_test is not.

@andygolay tagging you because I think you might have a better understanding of this.

andygolay commented 2 months ago

There's already a function MovementClient::new_for_test. I added it to provide a way to spin up a local Movement network, which is not needed on the Eth side because Eth has Anvil.

MovementClient::new() might not be fully fleshed out, for example there are dummy values in the current implementation. But there needs to be a production, non-test function for generating a new MovementClient, right? Isn't that the purpose of MovementClient::new?

EthClient::new is called in production (non-test) in the CLI

From what I can tell, the use for MovementClient::new hasn't been written into the CLI yet. It will need to be, and at that point we'll need MovementClient::new for production use.

It appears to me that the way @0xPrimata is addressing this issue, by annotating MovementClient::new_for_test and other test functions as shown here is correct.