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 deleteProposal() and missing PaymentAddress #178

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

JSKitty
Copy link
Member

@JSKitty JSKitty commented Aug 14, 2023

Abstract

This is a rather simple PR that fixes two critical bugs that can cause loss of funds and/or failure to submit or finalise a proposal on v1.1.0.

  • localProposals were missing PaymentAddress, leading to renderProposals failing, causing the entire Governance Dashboard to fail to render, and cascade errors up the stack.
  • deleteProposal() was also badly written: it filtered proposals that were NOT the target proposal, it had a txid typo using txId which never existed, and it was just generally unreadable, I've rewritten the function to fix the bugs and be far easier to review.

Testing

To test this PR, it's suggested to attempt these user flows, or variations of these:

  • Switch to Testnet.
  • Import / Create a wallet with at least 150 tPIV.
  • Create some proposals (in any speed or quantity you desire).
  • Finalise the proposals (this requires waiting 6 blocks for each proposal).

If all of the above, and variations of such; go without error, then things are looking bright!


What does this PR address?

It addresses two critical bugs that could cause loss of funds, as well as complete failure of MPW's Governance and Proposal Submission Dashboard, these were encountered by a mainnet user, and were verified this morning by myself on testnet.

@JSKitty JSKitty added Bug This is either a bugfix (PR) or a bug (issue). Awaiting Review This PR and/or issue is awaiting reviews before continuing. labels Aug 14, 2023
@JSKitty JSKitty requested review from Luke-Larsen, Liquid369, BreadJS and a team August 14, 2023 23:02
@JSKitty JSKitty self-assigned this Aug 14, 2023
Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

tACK 93ae2cd
Multiple proposals were created, spammed, and finalized.
No issues with the loss of funds, each was propagated successfully.
Reloaded and all seems well.

@JSKitty JSKitty added the Review Reward: 50 PIV Reviewers of this Pull Request will receive a 50 PIV reward label Aug 15, 2023
Copy link

@BreadJS BreadJS left a comment

Choose a reason for hiding this comment

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

Works amazing! LGTM! 💪

@JSKitty JSKitty merged commit 6882dc1 into master Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review This PR and/or issue is awaiting reviews before continuing. Bug This is either a bugfix (PR) or a bug (issue). Review Reward: 50 PIV Reviewers of this Pull Request will receive a 50 PIV reward
Projects
Development

Successfully merging this pull request may close these issues.

3 participants