[ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference #2892

stackman27 · 2022-09-29T01:42:41Z

Part of : #2579

What is the purpose of the change

This PR does the following;

Setup folder/file structure for the ValSet module
Setup KVStore for the module
Setup Keeper/Msg_server test suite
Create MsgSetValidatorSetPreference that lets users to create their validator-set
Make MsgSetValidatorSetPreference module so that it can handle create + update

Brief Changelog

n/a

Testing and Verifying

Added tdd tests for MsgSetValidatorSetPreference

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

x/validator-preference/keeper/msg_server.go

mattverse

Looking good! Left some initial comments, please take a look 🌮

x/validator-preference/keeper/keeper.go

x/validator-preference/valpref-module/module.go

go.mod

x/validator-preference/valpref-module/module.go

x/validator-preference/types/codec.go

x/validator-preference/types/msgs.go

x/validator-preference/keeper/ValidatorSet.go

x/validator-preference/keeper/msg_server.go

x/validator-preference/keeper/ValidatorSet.go

x/validator-preference/msg_server.go

czarcas7ic

Have a few changes, one in particular that is a must. Please lmk if you have any questions!

As a side note kind of unrelated to this particular PR, this concept of storing account valset preferences on chain is kind of disturbing to me. I could be wrong / misunderstanding, but wouldn't it be extremely simple for someone to spam create accounts and set validator preferences to preference all 150 validators? I don't even think the account would need to have any balance as it stands right? I think we need to rethink the implementation if this is the case

cc: @p0mvn @mattverse

proto/osmosis/validator-preference/v1beta1/params.proto

x/validator-preference/keeper_test.go

x/validator-preference/msg_server.go

x/validator-preference/msg_server_test.go

x/validator-preference/types/msgs.go

x/validator-preference/validator_set.go

stackman27 · 2022-10-05T22:36:11Z

Have a few changes, one in particular that is a must. Please lmk if you have any questions!

As a side note kind of unrelated to this particular PR, this concept of storing account valset preferences on chain is kind of disturbing to me. I could be wrong / misunderstanding, but wouldn't it be extremely simple for someone to spam create accounts and set validator preferences to preference all 150 validators? I don't even think the account would need to have any balance as it stands right? I think we need to rethink the implementation if this is the case

cc: @p0mvn @mattverse

Thank you @czarcas7ic for the feedback! Addressed everything here: bb8fb72

I do think we should have an upper limit for the # of validator we can have in a set. Yea the account donot need to have any balance to create a validator set. I can change it based on any design decision we end up making

czarcas7ic · 2022-10-05T22:51:11Z

I am going to tag @ValarDragon for his opinion on potential attack surface this would provide. I am not concerned about the number of validators an account can set. Even if it was two, if someone could create unlimited accounts with two validator preferences, this would bloat state significantly.

mattverse

Left second round of reviews, overall looking good, I think we just need test cases for the community pool funding for val set creation fee

go.mod

x/validator-preference/keeper.go

proto/osmosis/validator-preference/v1beta1/genesis.proto

x/validator-preference/keeper_test.go

x/validator-preference/msg_server.go

x/validator-preference/msg_server_test.go

x/validator-preference/valpref-module/module.go

mattverse · 2022-10-10T23:45:19Z

I am going to tag @ValarDragon for his opinion on potential attack surface this would provide. I am not concerned about the number of validators an account can set. Even if it was two, if someone could create unlimited accounts with two validator preferences, this would bloat state significantly.

My stance is that I'm not pretty much concerned about this, since we have the val set creation fee

czarcas7ic · 2022-10-10T23:56:20Z

Yes, as long as fee is implemented I am also not concerned

stackman27 · 2022-10-11T05:53:40Z

Left second round of reviews, overall looking good, I think we just need test cases for the community pool funding for val set creation fee

Thank you @mattverse for the feedback. Resolved all you feedback except replacing DeepEqual here 0c6ece9

I want to see if there is a much easier way of doing this other than sorting and comparing, maybe some built in function or osmo util function that i'm not aware of

mattverse · 2022-10-11T17:02:21Z

Left second round of reviews, overall looking good, I think we just need test cases for the community pool funding for val set creation fee

Thank you @mattverse for the feedback. Resolved all you feedback except replacing DeepEqual here 0c6ece9

I want to see if there is a much easier way of doing this other than sorting and comparing, maybe some built in function or osmo util function that i'm not aware of

Can you assign me for review once r4r?

czarcas7ic

My question on the validator_set.go is the only important point I had, all others are low pri, nice work :)

app/upgrades/v13/constants.go

x/valset-pref/msg_server_test.go

x/valset-pref/types/msgs_test.go

x/valset-pref/validator_set.go

x/valset-pref/valpref-module/module.go

app/keepers/keepers.go

x/valset-pref/valpref-module/module.go

app/keepers/keepers.go

app/modules.go

proto/osmosis/valset-pref/v1beta1/query.proto

proto/osmosis/valset-pref/v1beta1/state.proto

x/valset-pref/client/query_proto_wrap.go

x/valset-pref/keeper.go

x/valset-pref/msg_server.go

x/valset-pref/types/codec.go

stackman27 · 2022-11-02T00:22:06Z

Thank you @alexanderbez for your feedback! implemented all of it here c64682a

czarcas7ic

LGTM! Did we decide to limit the number of validators one can split delegate to? I dont have any preference for or against, I just remember this being talked about and I didn't see anything in ref to this in the code

x/valset-pref/types/msgs_test.go

czarcas7ic · 2022-11-03T16:14:45Z

@alexanderbez are you able to check if your required change was addressed so this can get merged?

stackman27 · 2022-11-03T21:52:41Z

LGTM! Did we decide to limit the number of validators one can split delegate to? I dont have any preference for or against, I just remember this being talked about and I didn't see anything in ref to this in the code

Dev seem to not care much about it so i didn't include it. If this discussion opens up in the future i will add it

alexanderbez

Left a few minor reviews. I think there are probably a few remaining spots that can be cleaned up. So I'll give this a preliminary ACK

x/valset-pref/types/codec.go

x/valset-pref/types/keys.go

x/valset-pref/types/msgs.go

x/valset-pref/client/query_proto_wrap.go

stackman27 · 2022-11-05T22:09:43Z

Thank you @alexanderbez resolved all your comments here a3a55cc

czarcas7ic · 2022-11-06T00:58:41Z

Merging, thanks again @stackman27 !

osmosis-labs#2892) * Module Wired up and created MsgSetValidatorSetPreference * [ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference * bez feedback

…reference (#2892)" This reverts commit 2ee66e4.

…atorSetPreference" (#3306) * Revert "[ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference (#2892)" This reverts commit 2ee66e4. * removed folder

github-actions bot added C:app-wiring Changes to the app folder C:CLI C:docs Improvements or additions to documentation labels Sep 29, 2022

stackman27 changed the title ~~[Val-SetPref] Module Wired up and created MsgSetValidatorSetPreference~~ [ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference Sep 29, 2022

stackman27 mentioned this pull request Sep 29, 2022

[EPIC: ValSet-Pref] Full Feature Implementation #2831

Closed

12 tasks

stackman27 commented Sep 29, 2022

View reviewed changes

x/validator-preference/keeper/msg_server.go Outdated Show resolved Hide resolved

stackman27 force-pushed the sis/valSet-setValMsg branch 2 times, most recently from f18a463 to 5a03823 Compare September 29, 2022 15:36

github-actions bot removed C:docs Improvements or additions to documentation C:CLI labels Sep 29, 2022

stackman27 force-pushed the sis/valSet-setValMsg branch from 11fec5c to 634f626 Compare September 29, 2022 17:17

stackman27 requested review from p0mvn, czarcas7ic and ValarDragon September 29, 2022 17:17

stackman27 marked this pull request as ready for review September 29, 2022 17:35

stackman27 requested review from a team and mattverse September 29, 2022 17:35

mattverse reviewed Sep 30, 2022

View reviewed changes

stackman27 commented Oct 2, 2022

View reviewed changes

x/validator-preference/msg_server.go Outdated Show resolved Hide resolved

czarcas7ic requested changes Oct 4, 2022

View reviewed changes

stackman27 force-pushed the sis/valSet-setValMsg branch from dae4663 to dcd44c5 Compare October 10, 2022 05:23

mattverse reviewed Oct 10, 2022

View reviewed changes

stackman27 requested review from mattverse and czarcas7ic October 11, 2022 21:45

stackman27 force-pushed the sis/valSet-setValMsg branch from a32cf1e to 623635a Compare October 31, 2022 20:43

czarcas7ic reviewed Oct 31, 2022

View reviewed changes

ValarDragon reviewed Nov 1, 2022

View reviewed changes

app/keepers/keepers.go Outdated Show resolved Hide resolved

alexanderbez reviewed Nov 1, 2022

View reviewed changes

stackman27 force-pushed the sis/valSet-setValMsg branch from e32b5d6 to c64682a Compare November 2, 2022 00:20

stackman27 requested review from czarcas7ic, alexanderbez and ValarDragon November 2, 2022 00:22

czarcas7ic approved these changes Nov 3, 2022

View reviewed changes

x/valset-pref/types/msgs_test.go Outdated Show resolved Hide resolved

stackman27 force-pushed the sis/valSet-setValMsg branch 2 times, most recently from 2a48057 to 091c2cc Compare November 4, 2022 05:01

stackman27 added 2 commits November 4, 2022 14:11

Module Wired up and created MsgSetValidatorSetPreference

4294b92

[ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference

892ae00

stackman27 force-pushed the sis/valSet-setValMsg branch from 091c2cc to 892ae00 Compare November 4, 2022 21:11

alexanderbez reviewed Nov 5, 2022

View reviewed changes

x/valset-pref/types/codec.go Show resolved Hide resolved

x/valset-pref/types/keys.go Outdated Show resolved Hide resolved

x/valset-pref/types/msgs.go Outdated Show resolved Hide resolved

x/valset-pref/types/msgs.go Outdated Show resolved Hide resolved

alexanderbez approved these changes Nov 5, 2022

View reviewed changes

x/valset-pref/client/query_proto_wrap.go Outdated Show resolved Hide resolved

bez feedback

a3a55cc

czarcas7ic merged commit 2ee66e4 into main Nov 6, 2022

czarcas7ic deleted the sis/valSet-setValMsg branch November 6, 2022 00:58

stackman27 added a commit that referenced this pull request Nov 8, 2022

Revert "[ValSet-Pref] Module Wired up and created MsgSetValidatorSetP…

9788ade

…reference (#2892)" This reverts commit 2ee66e4.

stackman27 mentioned this pull request Nov 8, 2022

Revert for v13 "[ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference" #3306

Merged

czarcas7ic mentioned this pull request Jan 5, 2023

chore: osmoutils, twap, downtime-detector, ibc-hooks cherry-pick to v14 #3925

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference #2892

[ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference #2892

stackman27 commented Sep 29, 2022 •

edited

Loading

mattverse left a comment •

edited

Loading

czarcas7ic left a comment

stackman27 commented Oct 5, 2022

czarcas7ic commented Oct 5, 2022

mattverse left a comment

mattverse commented Oct 10, 2022

czarcas7ic commented Oct 10, 2022

stackman27 commented Oct 11, 2022

mattverse commented Oct 11, 2022

czarcas7ic left a comment

stackman27 commented Nov 2, 2022

czarcas7ic left a comment

czarcas7ic commented Nov 3, 2022

stackman27 commented Nov 3, 2022

alexanderbez left a comment •

edited

Loading

stackman27 commented Nov 5, 2022

czarcas7ic commented Nov 6, 2022

[ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference #2892

[ValSet-Pref] Module Wired up and created MsgSetValidatorSetPreference #2892

Conversation

stackman27 commented Sep 29, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

mattverse left a comment • edited Loading

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

stackman27 commented Oct 5, 2022

czarcas7ic commented Oct 5, 2022

mattverse left a comment

Choose a reason for hiding this comment

mattverse commented Oct 10, 2022

czarcas7ic commented Oct 10, 2022

stackman27 commented Oct 11, 2022

mattverse commented Oct 11, 2022

czarcas7ic left a comment

Choose a reason for hiding this comment

stackman27 commented Nov 2, 2022

czarcas7ic left a comment

Choose a reason for hiding this comment

czarcas7ic commented Nov 3, 2022

stackman27 commented Nov 3, 2022

alexanderbez left a comment • edited Loading

Choose a reason for hiding this comment

stackman27 commented Nov 5, 2022

czarcas7ic commented Nov 6, 2022

stackman27 commented Sep 29, 2022 •

edited

Loading

mattverse left a comment •

edited

Loading

alexanderbez left a comment •

edited

Loading