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

Refactor/predicates include exclude #315

Merged
merged 11 commits into from
Mar 28, 2023
Merged

Conversation

ccamel
Copy link
Member

@ccamel ccamel commented Mar 20, 2023

Purpose

Implements #313. With this implementation, one can specifies a predicates_blacklist of predicates to exclude from the set of registered predicates that the Interpreter can use. In addition, theregistered_predicates parameter is renamed to predicates_whitelist for consistency.

Implementation details

  • The blacklist is always applied after the whitelist to ensure that blacklisted predicates prevail on whitelisted predicates.
  • The predicates are specified as a list of <predicate_name>/[<arity>] strings. If a predicate name without arity is included in this list, then all predicates with that name will be considered regardless of arity.

Consideration

⚠️ This PR introduces a breaking change due to the renaming of a property in the proto. The code that performs the migration v2 to v3 is included in the PR.

Todo

  • specify proto
  • implement feature
  • add tests
  • implement migration

@ccamel ccamel self-assigned this Mar 20, 2023
@ccamel ccamel force-pushed the refactor/predicates-include-exclude branch 5 times, most recently from f8b1984 to 5125ccb Compare March 20, 2023 17:39
@ccamel ccamel marked this pull request as ready for review March 20, 2023 17:48
@ccamel ccamel requested review from bdeneux and amimart March 20, 2023 17:49
@ccamel ccamel force-pushed the refactor/predicates-include-exclude branch from 5125ccb to 19717ce Compare March 22, 2023 12:40
@bdeneux
Copy link
Contributor

bdeneux commented Mar 27, 2023

@ccamel The migration wasn't working since paramspace is immediately initialized with the new params key table at the keeper instantiation before the migration run. Even with a legacy param subspace, we cannot change the registered key table (with the old one) at the migration step. So it impossible to get the old ParamsSet before migration to update it.

Otherwise the x/param module has been deprecated since 0.46.X so I took advantage of this migration to migrate also the ParamSet to move params into the module state. I've followed recommandation given in the cosmos changelog and the mint module PR given by cosmos. I've introduce the UpdateParams msg to give us the possibility of update module params by governance.

FYI @amimart.

@ccamel
Copy link
Member Author

ccamel commented Mar 27, 2023

@bdeneux This is excellent! Thanks for the improvement.

@bdeneux bdeneux force-pushed the refactor/predicates-include-exclude branch from aaa49f5 to f44d280 Compare March 27, 2023 15:24
Copy link
Member

@amimart amimart left a comment

Choose a reason for hiding this comment

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

Excellent! Everything seems fine 😉

Copy link
Contributor

@bdeneux bdeneux left a comment

Choose a reason for hiding this comment

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

Here my test to check if everything work 😜 :

  • query logic params (through CLI)
  • query logic ask (CLI)
  • migration from old x/param to module state and new renamed params
  • update new params through a "@type": "/logic.v1beta2.MsgUpdateParams" governance proposal

All good 👍 thank :)

@ccamel
Copy link
Member Author

ccamel commented Mar 28, 2023

All right! Thanks so much guys. ❤️

@ccamel ccamel merged commit dc32f9c into main Mar 28, 2023
@ccamel ccamel deleted the refactor/predicates-include-exclude branch March 28, 2023 08:26
@bot-anik
Copy link
Member

🎉 This PR is included in version 5.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants