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

feature: Structured parameters config #8384

Merged
merged 11 commits into from
Jan 19, 2023

Conversation

aborg-dev
Copy link
Contributor

@aborg-dev aborg-dev commented Jan 18, 2023

This is a part of #8264.

This PR introduces structured values for Rationals in the parameter config in the form

burnt_gas_reward: {
  numerator: 3,
  denominator: 10,
}

To support this in the code, we now interpret parameter values as one of

enum ParameterValue {
    Null,
    U64(u64),
    Rational { numerator: isize, denominator: isize },
    String(String),
}

In the future the plan is to extend this to support Fee structure and Weighted parameters.

One open Rust-related question here:

  • What is a good way to deduplicate code between get_number, get_string, get_rational methods?

Things to finish:

  • Tests for the Rational type

@aborg-dev aborg-dev requested a review from a team as a code owner January 18, 2023 12:09
@aborg-dev aborg-dev requested a review from jakmeier January 18, 2023 12:09
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Looks good! Almost ready to merge. Just a few nits and discussion points I left in comments.

core/primitives/src/runtime/parameter_table.rs Outdated Show resolved Hide resolved
core/primitives/src/runtime/parameter_table.rs Outdated Show resolved Hide resolved
core/primitives/src/runtime/parameter_table.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Looking good! Please change as_string to as_str as commented, otherwise this should be ready to merge.

core/primitives/src/runtime/parameter_table.rs Outdated Show resolved Hide resolved
core/primitives/src/runtime/parameter_table.rs Outdated Show resolved Hide resolved
@near-bulldozer near-bulldozer bot merged commit 82b2e99 into near:master Jan 19, 2023
nikurt pushed a commit to nikurt/nearcore that referenced this pull request Jan 30, 2023
This is a part of near#8264.

This PR introduces structured values for Rationals in the parameter config in the form
```yaml
burnt_gas_reward: {
  numerator: 3,
  denominator: 10,
}
```

To support this in the code, we now interpret parameter values as one of
```rust
enum ParameterValue {
    Null,
    U64(u64),
    Rational { numerator: isize, denominator: isize },
    String(String),
}
```

In the future the plan is to extend this to support Fee structure and Weighted parameters.

One open Rust-related question here:
- [ ] What is a good way to deduplicate code between `get_number`, `get_string`, `get_rational` methods?

Things to finish:
- [x] Tests for the Rational type
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