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

Fix governance vote purge #221

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from
Open

Fix governance vote purge #221

wants to merge 18 commits into from

Conversation

tequdev
Copy link
Collaborator

@tequdev tequdev commented Nov 28, 2023

High Level Overview of Change

Fixed previous_member and vote_key are set correctly when deleting votes in Governance Hook.

Context of Change

The implementation didn't match the top comment in the govern.c file.

  • L1/L2 information was missing when obtaining State.

Fixed existence comparison conditions.

Type of Change

  • [x ] Bug fix (non-breaking change which fixes an issue)

If each governance table continues to use this governance Hook, it is recommended that the Hook be updated.

Test Plan

Since the author is not familiar with C++ code and cannot write sufficient test code, so used JS for the test instead.

https://github.com/tequdev/xahau-governance-hook-test

hook/genesis/govern.c Outdated Show resolved Hide resolved
hook/genesis/govern.c Outdated Show resolved Hide resolved
hook/genesis/govern.c Outdated Show resolved Hide resolved
@RichardAH
Copy link
Contributor

I think this is correct but we really need a deterministic build setup for these "embedded" hooks so we can prove what the wasm binary contains. Not that I don't trust @tequdev, but because no one should have to trust an opaque binary to begin with. I feel uncomfortable about merging opaque binaries, even ones I generate myself.

@dangell7
Copy link
Collaborator

I think this is correct but we really need a deterministic build setup for these "embedded" hooks so we can prove what the wasm binary contains. Not that I don't trust @tequdev, but because no one should have to trust an opaque binary to begin with. I feel uncomfortable about merging opaque binaries, even ones I generate myself.

So you want to build and store the hex binary in the repo during the git actions? We could also do it how you did the tests where it shows both the c and the hex?

https://raw.githubusercontent.com/Xahau/xahaud/refs/heads/dev/src/test/app/SetHook_wasm.h

@RichardAH
Copy link
Contributor

I think this is correct but we really need a deterministic build setup for these "embedded" hooks so we can prove what the wasm binary contains. Not that I don't trust @tequdev, but because no one should have to trust an opaque binary to begin with. I feel uncomfortable about merging opaque binaries, even ones I generate myself.

So you want to build and store the hex binary in the repo during the git actions? We could also do it how you did the tests where it shows both the c and the hex?

https://raw.githubusercontent.com/Xahau/xahaud/refs/heads/dev/src/test/app/SetHook_wasm.h

Yeah I think automated building of the blobs during the git actions is the correct answer. But they have to be deterministic builds.

@RichardAH RichardAH force-pushed the dev branch 2 times, most recently from 77b8c83 to 532a471 Compare December 11, 2024 02:30
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.

Governance hook: Votes of removed members are not purged
4 participants