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 specific network/coin/balance types #619

Merged

Conversation

akildemir
Copy link
Contributor

This pr adds new types such as ExternalNetworkId, ExternalCoin to distinctly identify them and help reduce the bugs. These changes helps code readability and prevent us from constantly checking for native network or coin.

Copy link
Member

@kayabaNerve kayabaNerve left a comment

Choose a reason for hiding this comment

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

Really great catch on what we should do. Bunch of nits which honestly don't matter, yet the wire-format comments are worth discussing!

coordinator/src/cosign_evaluator.rs Outdated Show resolved Hide resolved
coordinator/src/substrate/mod.rs Outdated Show resolved Hide resolved
coordinator/src/substrate/mod.rs Outdated Show resolved Hide resolved
coordinator/src/substrate/mod.rs Outdated Show resolved Hide resolved
substrate/coins/pallet/src/lib.rs Show resolved Hide resolved
substrate/primitives/src/networks.rs Outdated Show resolved Hide resolved
substrate/primitives/src/networks.rs Outdated Show resolved Hide resolved
substrate/primitives/src/networks.rs Outdated Show resolved Hide resolved
substrate/primitives/src/networks.rs Outdated Show resolved Hide resolved
substrate/primitives/src/networks.rs Outdated Show resolved Hide resolved
let desired_stake = total_stake / (100 / SERAI_VALIDATORS_DESIRED_PERCENTAGE);
if stake >= desired_stake {
Err(Error::<T>::NetworkHasEconomicSecurity)?;
}
Copy link
Member

Choose a reason for hiding this comment

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

Hm. I'm fine kicking this to a separate PR if we can't immediately resolve it, yet I do want to discuss this algorithm more. This limits to 20% of the current stake to external networks. That works if and only if Serai is the last network to be staked for. It does not allow Serai to be the first algorithm to be staked for.

From the initial distribution, we can calculate how much the Serai network should have and simply use that value as the limit.

That has the following quirks in behavior:

  1. It allows the Serai set to be staked for at an arbitrary point in time, with a much simpler check here
  2. If various LPs leave (burning the SRI, reducing the amount needed for economic security), then we may have more SRI stake than targeted

In the case of 2, we simply revert to relying on the emissions distribution to maintain the optimal distribution. I do have a concern where a large amount of people stake to the SRI network just to swap to SRI at the spot price. While we're discussing non-trivial slashes re: external networks for offline validators, running a Serai node may only have trivial slashes and is far less annoying to setup a node for. Then since the Serai network is perceived as having infinite capacity, validators can immediately unstake after economic security with their spot-price SRI. The first option (currently implemented) does better in that regard by ensuring the amount staked to SRI is only the amount needed.

Also, further edge case, the final stake which causes economic security is unbounded. This is not too likely to be an issue? But likely yet another reason to open an issue for this and kick it to another PR (and if we do that, I'm fine leaving this here as the initial implementation).

substrate/dex/pallet/src/tests.rs Show resolved Hide resolved
@kayabaNerve kayabaNerve merged commit 435f1d9 into serai-dex:develop Oct 7, 2024
16 of 18 checks passed
@akildemir akildemir deleted the specify-external-networks-coins branch October 7, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants