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

Change by-value to by-reference to persist vote #4256

Closed
wants to merge 1 commit into from

Conversation

HowardHinnant
Copy link
Contributor

@HowardHinnant HowardHinnant commented Jul 27, 2022

High Level Overview of Change

Simple bug fix for persisting votes.

Context of Change

This is a fix to a recent fix. It simply changes a value variable to a reference variable. I missed this because I only tested an amendment with a default vote of yes, and the bug only is exposed only with amendments with a default vote of no.

Type of Change

  • [ x] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Test Plan

This was tested by starting rippled and issuing the following commands on the command line:

rippled feature

Observe some amendment with a default vote of no, such as NonFungibleTokensV1_1, is vetoed.

Unveto the amendment with:

rippled feature NonFungibleTokensV1_1 accept

Stop rippled:

rippled stop

Restart rippled and:

rippled feature

Note that the amendment now is listed with "vetoed" : false.

Amendments with a default vote of no can be found in the source file: src/ripple/protocol/impl/Feature.cpp.

Copy link
Contributor

@greg7mdp greg7mdp left a comment

Choose a reason for hiding this comment

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

lgtm. Just a possible future improvement thought - although I didn't investigate - maybe AmendmentState should not be copyable to avoid these kind of issues?

@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 3, 2022
This was referenced Aug 4, 2022
@nbougalis nbougalis mentioned this pull request Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants