Conversation
chong-he
added
enhancement
macladson
left a comment
macladson
left a comment
There was a problem hiding this comment.
Thanks for the PR! Looks like this needs a quick cargo fmt --all
Some of the tests don't seem happy either
macladson
added
work-in-progress
|
I'll do that, as well as take a look at the tests. Appreciate the review! |
| pub fn validate_extra_fields(&self) -> Result<(), String> { | ||
| for key in self.future_fields.keys() { | ||
| if !FUTURE_FIELDS.contains(&key.as_str()) { | ||
| return Err(format!("Unknown or disallowed field found: {}", key)); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I like this approach. Simple and flexible.
Let me know if you get stuck fixing any of the tests
|
Alright, I fixed up the tests. Most of the issues were simply because the validation and removal of defaults meant I had to add the missing fields in the existing configs. For this, I ended up using the original default values to maintain the same functionality as before. |
|
Some required checks have failed. Could you please take a look @Patchoulis? 🙏 |
mergify
bot
added
the
waiting-on-author
|
Hi @Patchoulis, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time. |
mergify
bot
added
the
stale
|
Hi @Patchoulis, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time. |
5 participants
Issue Addressed
#6208
Proposed Changes
A few changes. First, as discussed in the discord, the defaults have been removed to ensure that all expected fields are provided.
Second, I do validate that no additional values are present. However, as discussed with sproul, there are some legitimate reasons to add some for future forks. I ended up solving this by adding a small modifiable constant which allows specified optional fields to exist.
I'm doing some tests to ensure presets like SLOTS_PER_EPOCH are also disallowed, but this should also solve that problem.
Additional Info
Please provide any additional information. For example, future considerations
or information useful for reviewers.