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

feat: add new privacy type #954

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

feat: add new privacy type #954

wants to merge 12 commits into from

Conversation

ChaituVR
Copy link
Member

@ChaituVR ChaituVR commented Oct 31, 2024

Summary

  • Adds new option in space settings to select "any" privacy type
  • Adds new option on proposal editor to enable/disable shielded voting
  • Adds new option on update proposal editor to enable/disable shielded voting

Closes: https://github.com/snapshot-labs/workflow/issues/277

How to test

  1. Go to your settings, under Privacy - you should see the new option "any"
Untitled 4
  1. Inside proposal editor, you should see new option
    image
image
  1. Try create a new proposal with/without shutter
  2. Try updating proposal with/without shutter

@ChaituVR
Copy link
Member Author

Comments:

  • Default to Any
  • Any should be the first option in the list

@bonustrack
Copy link
Member

Just tested and the design looks good

@ChaituVR ChaituVR changed the title [Design check] feat: add new privacy type feat: add new privacy type Nov 27, 2024
@ChaituVR ChaituVR marked this pull request as ready for review November 30, 2024 06:21
@ChaituVR ChaituVR requested review from Copilot, Sekhmet and wa0x6e and removed request for Copilot and Sekhmet November 30, 2024 06:21

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 20 changed files in this pull request and generated no suggestions.

Files not reviewed (15)
  • apps/ui/src/components/FormSpaceVoting.vue: Evaluated as low risk
  • apps/ui/src/components/Modal/SelectPrivacy.vue: Evaluated as low risk
  • apps/ui/src/views/Space/Editor.vue: Evaluated as low risk
  • apps/ui/src/views/Proposal/Overview.vue: Evaluated as low risk
  • apps/ui/src/composables/useActions.ts: Evaluated as low risk
  • packages/sx.js/src/types/index.ts: Evaluated as low risk
  • packages/sx.js/src/clients/offchain/types.ts: Evaluated as low risk
  • apps/ui/src/helpers/constants.ts: Evaluated as low risk
  • packages/sx.js/src/clients/offchain/ethereum-sig/types.ts: Evaluated as low risk
  • apps/ui/src/networks/starknet/actions.ts: Evaluated as low risk
  • apps/ui/src/networks/types.ts: Evaluated as low risk
  • apps/ui/src/networks/offchain/api/index.ts: Evaluated as low risk
  • apps/ui/src/networks/evm/actions.ts: Evaluated as low risk
  • apps/ui/src/networks/common/graphqlApi/index.ts: Evaluated as low risk
  • apps/ui/src/networks/offchain/actions.ts: Evaluated as low risk
Comments skipped due to low confidence (1)

apps/ui/src/composables/useSpaceSettings.ts:164

  • [nitpick] The use of an empty string ('') as a valid value for the privacy field might be ambiguous. Consider using a more descriptive value or ensure that its use is well-documented.
const privacy = ref<'' | 'shutter' | 'any'>('');
Copy link
Member

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

It look good to me, maybe only thing that I'd change is avoid using "" as a value with meaning (like before we had "none" for this that got converted at the edge when storing/retrieving settings). Any reason we ended up introducing it (other than to make it match offchain space settings)?

I'd rather keep clearer values like "none" | "any" | "shutter".

@@ -50,6 +50,7 @@ describe('EthereumSig', () => {
discussion: 'https://snapshot.org',
choices: ['For', 'Against', 'Abstain'],
labels: ['1234e'],
privacy: 'shutter',
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead od modifying existing test case we should add new one for shutter privacy (the rest can stay the same).

@@ -128,6 +128,7 @@ export function useEditor() {
discussion: '',
type,
choices,
privacy: '',
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it doesn't matter there, but if space has configured Shielded voting privacy, and you submit proposal privacy will be ''.

I tested it and proposal still ends up using Shutter, but it's a bit confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

with out privacy we need to pass this type on a condition 🙈 will add a layer of confusion

maybe we should just pass shutter by default if space uses shutter?

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.

3 participants