-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adding support for multiple devnet relayers #45
Conversation
…dinal-Cryptography/membrane-bridge into A0-3726-enable-multiple-dev-relayers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added one suggestion (in two parts), but maybe it should be a separate PR.
@@ -72,10 +73,14 @@ fn main() -> Result<()> { | |||
// If no keystore path is provided, we use the default development mnemonic | |||
MnemonicBuilder::<English>::default() | |||
.phrase(DEV_MNEMONIC) | |||
.index(config.dev_account_index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Make it explicit that we're running in dev mode (some flag like --dev
) and crash with a descriptive error if neither --dev
nor --eth-keystore-path
is set.
relayer/src/main.rs
Outdated
let keypair = aleph_client::keypair_from_string(&config.azero_sudo_seed); | ||
let azero_seed = "//".to_owned() + &config.dev_account_index.to_string(); | ||
let azero_keypair = aleph_client::keypair_from_string(&azero_seed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: same deal as with eth - require either --dev
or --azero-key-seed
.
Update: added a compose file for running 3 bridgenet relayers + |
relayer/src/main.rs
Outdated
|
||
let azero_connection = Arc::new(azero::sign( | ||
&azero::init(&config.azero_node_wss_url).await, | ||
&azero_keypair, | ||
)); | ||
|
||
let wallet = if !config.eth_keystore_path.is_empty() { | ||
let wallet = if !config.dev { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: flip the if
so that it's in the positive - it should be easier to read. if config.dev { /* do dev stuff */ } else { /* do non-dev stuff */ }
.
Makefile
Outdated
bridgenet-bridge: # Run the bridge on bridgenet | ||
bridgenet-bridge: build-docker-relayer redis-instance | ||
NETWORK=bridgenet AZERO_ENV=bridgenet make deploy | ||
docker-compose -f ./relayer/scripts/bridgenet-relayers-compose.yml up -d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: use docker compose
instead of docker-compose
. I think it should be more portable? At least on my machine there is no docker-compose
command, while docker compose
works.
@@ -18,7 +18,7 @@ function get_address { | |||
|
|||
# --- ARGS | |||
|
|||
ETH_NETWORK=${ETH_NETWORK:-"ws://127.0.0.1:8546"} | |||
ETH_NETWORK=${ETH_NETWORK:-"http://127.0.0.1:8546"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why http
instead of ws
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not able to connect through wss
to our bridgenet node, so now relayer only talks to specified eth node via http
.
I could try to generalize it, so that relayer can accept both ws
and http
, if you think it's worth it.
Adds config field that is an index of a key derived from our development mnemonic.
Additionally, modifies eth dev chainspec so that corresponding accounts have funds required for relayer operation.
Note: when running 3 relayers some issues with wrap/unwrap showed, but I think fixing them should be done in later PRs