Skip to content

Conversation

wa0x6e
Copy link
Contributor

@wa0x6e wa0x6e commented Dec 14, 2024

Summary

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

This PR adds support for offchain proposals with pending results (when scores_state is not final)

Screenshot 2024-12-15 at 02 29 56

Pending score happens for:

  • proposal with privacy, when choices are being decrypted on proposal close
  • proposal with overriding strategies, which recompute results on proposal close

How to test

On proposal with privacy

  1. Go to http://localhost:8080/#/s:192022.eth/proposal/0xcf442aa4277322dd14715326541d43c9c9bd7bf143d3bebfc93ed685a45c2668/votes
  2. It should display a message about results being finalized in place of the results (in the sidebar)
  3. It should display a message in place of the choice in the votes list

On proposal without privacy

  1. Go to http://localhost:8080/#/s:aave.eth/proposal/0x95a80ac25f97e0fc06528452e2c33d7cc179b801ddc4d8dd164964ce8e1d450e/votes
  2. It should display a message about results being finalized in place of the results (in the sidebar)
  3. The choice in the votes list should be the final choice

Note

We don't support when the proposal has scores_state as invalid state, since it's not handled on v1 either

@wa0x6e wa0x6e marked this pull request as ready for review December 14, 2024 18:24
@wa0x6e wa0x6e force-pushed the show-pending-results-message branch from 0eca2bc to cacab7b Compare December 14, 2024 18:27
@wa0x6e wa0x6e force-pushed the show-pending-results-message branch from cacab7b to 816e89e Compare December 14, 2024 18:28
@bonustrack
Copy link
Member

bonustrack commented Dec 16, 2024

Any idea why these proposal are stuck with results not finalized? Is just proposal with invalid score?

@bonustrack
Copy link
Member

We should show the logo Shutter on proposals with Privacy

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Dec 16, 2024

Proposals are stuck for various reasons:

There's currently ~260 closed proposals with pending state. Will trigger the score finalization manually.

We should have a somewhere to ensure the finalization is triggered in case webhook is lost

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.

Looks good to me, how do we trigger webhook? I visited your proposal on v1 to see how it's handled there and it looks like it triggered decryption.

Do we trigger it from the UI? If that's the case then we probably should bring back that trigger to new UI, otherwise we won't trigger it 🤔

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Dec 18, 2024

Looks good to me, how do we trigger webhook? I visited your proposal on v1 to see how it's handled there and it looks like it triggered decryption.

Do we trigger it from the UI? If that's the case then we probably should bring back that trigger to new UI, otherwise we won't trigger it 🤔

Computation is triggered via webhook, on proposal close, everything is done on backend, no more trigger for that from the UI.

There's a few cases where finalization trigger are missed, as webhook are not retried on error, need to work on a solution to ensure all trigger is called.

@wa0x6e wa0x6e requested a review from Sekhmet December 18, 2024 17:07
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.

tACK

@Sekhmet Sekhmet merged commit cb1245e into master Dec 19, 2024
2 checks passed
@Sekhmet Sekhmet deleted the show-pending-results-message branch December 19, 2024 14:43
Sekhmet added a commit that referenced this pull request Jun 30, 2025
Originally `completed` was used only for onchain spaces and was used for
execution, but later on it was reused for pending results
(#1055).

This meant that it started carrying two very different meanings and
broke things, so later on completed for onchain spaces was updated
to match offchain space meaning
(#1075) which broke
onchain execution.
Sekhmet added a commit that referenced this pull request Jun 30, 2025
Originally `completed` was used only for onchain spaces and was used for
execution, but later on it was reused for pending results
(#1055).

This meant that it started carrying two very different meanings and
broke things, so later on completed for onchain spaces was updated
to match offchain space meaning
(#1075) which broke
onchain execution.
Sekhmet added a commit that referenced this pull request Jul 1, 2025
* fix: separate completed and execution_settled

Originally `completed` was used only for onchain spaces and was used for
execution, but later on it was reused for pending results
(#1055).

This meant that it started carrying two very different meanings and
broke things, so later on completed for onchain spaces was updated
to match offchain space meaning
(#1075) which broke
onchain execution.

* chore: add execution_settled field

This will replace completed in the future. Renaming fields in API
are annoying because we need to carry two in the API during migration.
For now we are just adding new field, but still using old one in UI.

In the future next time we update API we can just remove completed field
and start using execution_settled on UI.
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