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

Add chain parameters for the Ethereum bridge #575

Merged
merged 12 commits into from
Oct 24, 2022

Conversation

james-chf
Copy link
Contributor

@james-chf james-chf commented Oct 10, 2022

Relates to #365 and #562

This PR adds preliminary chain parameters for the Ethereum bridge, and also renames ledger runtime configuration to be under [ledger.ethereum_bridge] rather than just [ledger.ethereum]. We can change/add extra validation for these configuration parameters in other PRs.

In a network config file, Ethereum bridge parameters will be able to be specified like so:

[ethereum_bridge_params]
min_confirmations = 100

[ethereum_bridge_params.contracts]
native_erc20 = "0x1721b337BBdd2b11f9Eef736d9192a8E9Cba5872"

[ethereum_bridge_params.contracts.bridge]
address = "0x237d915037A1ba79365E84e2b8574301B6D25Ea0"
version = 1

[ethereum_bridge_params.contracts.governance]
address = "0x308728EEa73538d0edEfd95EF148Eb678F71c71D"
version = 1

These parameters as a whole are optional but if any are set, then all must be set. i.e. a Namada chain can either start without any Ethereum bridge parameters set, or with all Ethereum bridge parameters set.

This PR only adds the parameter fields, but they aren't used yet by the oracle. When validator nodes start up, they will need to check blockchain storage for the appropriate parameter values for whatever epoch they have synced to, and configure their events oracle accordingly. If no Ethereum bridge parameters have been set for a chain yet, then validator nodes don't need to start any Ethereum bridge componentry (i.e. the events oracle or geth).

Specs need to be updated for no more proxy contract (have added an item to #448)

I've tested this manually and have been able to namadac utils init-network etc. with this network config file which has Ethereum bridge parameters: network-config.toml.txt

@james-chf james-chf force-pushed the james/ethbridge/config branch 2 times, most recently from 27cc639 to 15175ee Compare October 14, 2022 13:10
@james-chf james-chf force-pushed the james/ethbridge/config branch 2 times, most recently from 7b82276 to 3cbe061 Compare October 20, 2022 14:47
@james-chf james-chf changed the title Add genesis parameters for the Ethereum bridge Add chain parameters for the Ethereum bridge Oct 20, 2022
@james-chf james-chf marked this pull request as ready for review October 20, 2022 17:51
apps/src/lib/config/ethereum_bridge/params.rs Outdated Show resolved Hide resolved
apps/src/lib/config/ethereum_bridge/ledger.rs Outdated Show resolved Hide resolved
apps/src/lib/config/ethereum_bridge/ledger.rs Outdated Show resolved Hide resolved
apps/src/lib/config/ethereum_bridge/params.rs Outdated Show resolved Hide resolved
apps/src/lib/config/ethereum_bridge/params.rs Outdated Show resolved Hide resolved
tests/src/e2e/setup.rs Outdated Show resolved Hide resolved
@james-chf james-chf marked this pull request as draft October 21, 2022 09:56
@james-chf james-chf marked this pull request as ready for review October 21, 2022 10:04
@james-chf james-chf requested a review from sug0 October 21, 2022 10:04
@james-chf james-chf force-pushed the james/ethbridge/config branch from d5d7377 to a284afc Compare October 21, 2022 12:12
@james-chf james-chf requested a review from sug0 October 21, 2022 12:15

/// Represents chain parameters for the Ethereum bridge.
#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)]
pub struct Config {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub struct Config {
pub struct GenesisConfig {

Copy link
Member

Choose a reason for hiding this comment

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

Confused me at first because I though this was something that could be configured when starting a node. Reading the later code, I realized that wasn't true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have renamed to GenesisConfig

// SAFETY: The only way the API contract of `NonZeroU64` can be
// violated is if we construct values of this type using 0 as
// argument.
Self(unsafe { NonZeroU64::new_unchecked(1) })
Copy link
Member

Choose a reason for hiding this comment

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

Amazes me that this type does not implement Default.

@@ -83,7 +83,7 @@ pub struct Ledger {
pub chain_id: ChainId,
pub shell: Shell,
pub tendermint: Tendermint,
pub ethereum: ethereum::Config,
pub ethereum_bridge: ethereum_bridge::ledger::Config,
Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, you do have this as a config. We can't configure minimum confirmations or addresses in a normal config. They have to be set at genesis and changed only with governance votes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah okay that makes sense, since you can run into consensus failures if two validators have different ideas of what the min number of blocks should be for an event to be considered as valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the Tendermint v0.37 design, we shouldn't get consensus failures if validators have different ideas of min confirmations. For the addresses of the contracts, those should change only via hard fork as far as I understand it, if we do a contract upgrade, though technically it would be possible for a a governance proposal to change them (right now as the code currently is).

@james-chf james-chf force-pushed the james/ethbridge/config branch from fbcec89 to 65c80a6 Compare October 24, 2022 13:07
@james-chf james-chf requested a review from batconjurer October 24, 2022 13:19
@james-chf james-chf merged commit b3c470c into eth-bridge-integration Oct 24, 2022
@james-chf james-chf deleted the james/ethbridge/config branch October 24, 2022 17:21
phy-chain pushed a commit to phy-chain/namada that referenced this pull request Mar 1, 2024
It was failing because there was no chain config present in the store.

Fixes anoma#575.
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.

3 participants