Slot Key Verification #121

Orland0x · 2022-05-11T23:09:41Z

Added slot key verification to the single slot strategy so that users can only submit proofs for the correct slots.

To do this I had to add another parameter called global_voting_strategy_params which is an array stored for each voting strategy set in the space. global_voting_strategy_params has broader use case: it is fully general purpose and can be used to customize any voting strategy defined with specific parameters.

For single slot proof, global_voting_stategy_params is structured as follows:

global_voting_stategy_params[0] = contract address where storage is read from. (eg ERC20 token contract address)
global_voting_stategy_params[1] = slot index of the mapping where storage is being read from.

Also added a new data structure called Immutable2DArray which is used to hold global_voting_strategy_params and voting_strategy_params for all strategies (array of arrays type structure)

pscott

Solid! Just a couple of nitpicks here and there.
One thing I would like to change though is naming! I think we need to come up with better naming, as I had a tough time differentiating global_params vs global_params_all etc etc.

To make sure I have a good understanding, what we have now is:

voting_strategy_params: user-defined array of data
global_params: voting strategy specific array, recorded in the global_voting_strategy_params storage var
global_params_all and voting_strategy_params_all: a 2d immutable array consisting of all the different global_params. (voting_strategy_params_all is used in get_cumulative_voting_power and global_params_all in register_global_voting_strategy_params)

Is that correct?

contracts/starknet/interfaces/i_voting_strategy.cairo

contracts/starknet/lib/array2d.cairo

contracts/starknet/lib/felt_to_uint256.cairo

contracts/starknet/lib/slot_key.cairo

contracts/starknet/voting_strategies/single_slot_proof.cairo

test/starknet/lib_array2d.ts

contracts/starknet/voting_strategies/single_slot_proof.cairo

Orland0x · 2022-05-17T10:21:59Z

Solid! Just a couple of nitpicks here and there. One thing I would like to change though is naming! I think we need to come up with better naming, as I had a tough time differentiating global_params vs global_params_all etc etc.

To make sure I have a good understanding, what we have now is:

voting_strategy_params: user-defined array of data

global_params: voting strategy specific array, recorded in the global_voting_strategy_params storage var

global_params_all and voting_strategy_params_all: a 2d immutable array consisting of all the different global_params. (voting_strategy_params_all is used in get_cumulative_voting_power and global_params_all in register_global_voting_strategy_params)

Is that correct?

yeah this is what i used. i used params and global_params when in the frame of reference of the voting strategy (as voting strategy is implied) and voting_strategy_params and global_voting_strategy_params params when in the space contract frame.

contracts/starknet/voting_strategies/single_slot_proof.cairo

pscott · 2022-05-18T16:25:16Z

Solid! Just a couple of nitpicks here and there. One thing I would like to change though is naming! I think we need to come up with better naming, as I had a tough time differentiating global_params vs global_params_all etc etc.
To make sure I have a good understanding, what we have now is:

voting_strategy_params: user-defined array of data

global_params: voting strategy specific array, recorded in the global_voting_strategy_params storage var

global_params_all and voting_strategy_params_all: a 2d immutable array consisting of all the different global_params. (voting_strategy_params_all is used in get_cumulative_voting_power and global_params_all in register_global_voting_strategy_params)

Is that correct?

yeah this is what i used. i used params and global_params when in the frame of reference of the voting strategy (as voting strategy is implied) and voting_strategy_params and global_voting_strategy_params params when in the space contract frame.

What would you think about remove strategy (to make it less verbose) and use:

user_voting_params: user-defined array of data
space_voting_params: (or could be protocol_voting_params or strategy_voting_params or constant_voting_params. I think global sounds like it's common for all voting strategies?)

And maybe keeping the same naming for space / voting_strategies context, as it makes it easier to navigate the code and not get lost. Not sure about this last one, just my thoughts :)

Orland0x · 2022-05-18T18:04:44Z

Solid! Just a couple of nitpicks here and there. One thing I would like to change though is naming! I think we need to come up with better naming, as I had a tough time differentiating global_params vs global_params_all etc etc.
To make sure I have a good understanding, what we have now is:

voting_strategy_params: user-defined array of data

global_params: voting strategy specific array, recorded in the global_voting_strategy_params storage var

global_params_all and voting_strategy_params_all: a 2d immutable array consisting of all the different global_params. (voting_strategy_params_all is used in get_cumulative_voting_power and global_params_all in register_global_voting_strategy_params)

Is that correct?

yeah this is what i used. i used params and global_params when in the frame of reference of the voting strategy (as voting strategy is implied) and voting_strategy_params and global_voting_strategy_params params when in the space contract frame.

What would you think about remove strategy (to make it less verbose) and use:

user_voting_params: user-defined array of data

space_voting_params: (or could be protocol_voting_params or strategy_voting_params or constant_voting_params. I think global sounds like it's common for all voting strategies?)

And maybe keeping the same naming for space / voting_strategies context, as it makes it easier to navigate the code and not get lost. Not sure about this last one, just my thoughts :)

I think its important we have strategy in the name as otherwise its not clear that they refer to parameters for voting strategies. Snapshot X is a voting protocol after all, so voting_params could conceivably refer to many different things.

yeah user_voting_params makes sense I agree.

hmmmm i see your point. Although to me, space_voting_params sounds more like it refers to something common to all strategies than global_voting_params does. Ill try to think of a better name...

yep i can keep same names with the space/voting strategy context.

I think all of these ambiguities will be solved if we have a good natspec standard frankly

Orland0x · 2022-05-18T18:12:06Z

we could do user_voting_strategy_params and then just voting_strategy_params for the global ones?

pscott · 2022-05-18T18:19:41Z

we could do user_voting_strategy_params and then just voting_strategy_params for the global ones?

yes! There are three different variables though: the user defined, the storage_var (which takes all voting_strategy_parameters and what the voting_strategy_parameters storage_var holds.
Maybe we could do

user_voting_strategy_params
voting_strategies_params (for the storage var?)
and the voting_strategies_params would store some voting_strategy_params ?

The only issue I see is that it's hard to differentiate... or we could also do

user_voting_strategy_params
global_voting_strategy_params for the storage var
and the storage var would store some voting_strategy_params

? wdyt?

Orland0x · 2022-05-18T18:46:26Z

we could do user_voting_strategy_params and then just voting_strategy_params for the global ones?

yes! There are three different variables though: the user defined, the storage_var (which takes all voting_strategy_parameters and what the voting_strategy_parameters storage_var holds. Maybe we could do

user_voting_strategy_params

voting_strategies_params (for the storage var?)

and the voting_strategies_params would store some voting_strategy_params ?

The only issue I see is that it's hard to differentiate... or we could also do

user_voting_strategy_params

global_voting_strategy_params for the storage var

and the storage var would store some voting_strategy_params

? wdyt?

why not just voting_strategy_params_store for the storage var? tbh we could just make the store suffix a standard for every storage var in the contract. seems to be what starkware do anyway

pscott · 2022-05-19T09:24:55Z

why not just voting_strategy_params_store for the storage var? tbh we could just make the store suffix a standard for every storage var in the contract. seems to be what starkware do anyway

I haven't seen this "standard" but i think it would be an excellent idea indeed. Should it be store or storage? (store sounds like it could be interpreted as a verb / action?)

pscott · 2022-05-19T09:26:31Z

I know I'm being a little "picky" here, but I do think that we need to be careful about those as we will probably be setting the standard for the rest of the codebase and the future, and naming is an important and difficult task :p

After all, as Phil Karlton said, There are only two hard things in Computer Science: cache invalidation and naming things. !

Orland0x · 2022-05-19T09:38:56Z

why not just voting_strategy_params_store for the storage var? tbh we could just make the store suffix a standard for every storage var in the contract. seems to be what starkware do anyway

I haven't seen this "standard" but i think it would be an excellent idea indeed. Should it be store or storage? (store sounds like it could be interpreted as a verb / action?)

cool lets do that then, store is generally used as a noun (like 'im going to store to get some eggs') so i think it makes sense slightly more sense to use here

Orland0x · 2022-05-19T09:39:44Z

I know I'm being a little "picky" here, but I do think that we need to be careful about those as we will probably be setting the standard for the rest of the codebase and the future, and naming is an important and difficult task :p

After all, as Phil Karlton said, There are only two hard things in Computer Science: cache invalidation and naming things. !

completely agree, crucial we get this space contract right.

Orland0x · 2022-05-22T16:12:15Z

think all the fixes are in

pscott

💪

pscott · 2022-05-24T09:50:49Z

contracts/starknet/space.cairo

+# NOTE: We need to think carefully about how to handle the case where a voting strategy is removed.
+# Do we also need to remove the voting strategy params? Currently we are not.
+# Setting the voting strategy to 0 will remove it from the store, but what happens if someone re-adds the strategy but with different params?


Btw I think we should indeed remove the associated voting strategy params :)

Orland0x added 5 commits May 10, 2022 17:53

merged files

73fce2f

merged space from develop

487d963

updated tests

1e826de

single slot test with slot key verif

ccc1515

formatting

dcd9914

Orland0x marked this pull request as draft May 11, 2022 23:10

Orland0x added 5 commits May 12, 2022 11:07

fixed more tests

0170b1e

fixed more tests

7441f0f

single slot strategy added

59d2ca0

merged quorum stuff

cc91a0d

merged quorum stuff

51784ac

Orland0x marked this pull request as ready for review May 13, 2022 12:17

pscott self-requested a review May 13, 2022 12:34

pscott requested changes May 17, 2022

View reviewed changes

Orland0x added 2 commits May 17, 2022 15:44

some fixes

1219252

formatting

f6aa232

pscott reviewed May 18, 2022

View reviewed changes

contracts/starknet/voting_strategies/single_slot_proof.cairo Show resolved Hide resolved

pscott reviewed May 18, 2022

View reviewed changes

contracts/starknet/voting_strategies/single_slot_proof.cairo Outdated Show resolved Hide resolved

Orland0x added 2 commits May 22, 2022 16:06

added store suffix to storage vars

dad591e

fixed test interfaces

2e2bfca

fixed whitelist test

4543e94

pscott approved these changes May 24, 2022

View reviewed changes

pscott reviewed May 24, 2022

View reviewed changes

Orland0x merged commit 52b2f15 into develop May 24, 2022

Orland0x deleted the verify_slot_key branch May 28, 2022 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slot Key Verification #121

Slot Key Verification #121

Orland0x commented May 11, 2022 •

edited

Loading

pscott left a comment

Orland0x commented May 17, 2022 •

edited

Loading

pscott commented May 18, 2022

Orland0x commented May 18, 2022

Orland0x commented May 18, 2022 •

edited

Loading

pscott commented May 18, 2022

Orland0x commented May 18, 2022 •

edited

Loading

pscott commented May 19, 2022

pscott commented May 19, 2022

Orland0x commented May 19, 2022

Orland0x commented May 19, 2022

Orland0x commented May 22, 2022

pscott left a comment

pscott May 24, 2022

Slot Key Verification #121

Slot Key Verification #121

Conversation

Orland0x commented May 11, 2022 • edited Loading

pscott left a comment

Choose a reason for hiding this comment

Orland0x commented May 17, 2022 • edited Loading

pscott commented May 18, 2022

Orland0x commented May 18, 2022

Orland0x commented May 18, 2022 • edited Loading

pscott commented May 18, 2022

Orland0x commented May 18, 2022 • edited Loading

pscott commented May 19, 2022

pscott commented May 19, 2022

Orland0x commented May 19, 2022

Orland0x commented May 19, 2022

Orland0x commented May 22, 2022

pscott left a comment

Choose a reason for hiding this comment

pscott May 24, 2022

Choose a reason for hiding this comment

Orland0x commented May 11, 2022 •

edited

Loading

Orland0x commented May 17, 2022 •

edited

Loading

Orland0x commented May 18, 2022 •

edited

Loading

Orland0x commented May 18, 2022 •

edited

Loading