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

Undo API changes introduced in XRPFees: #4429

Merged
merged 3 commits into from
Feb 24, 2023
Merged

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Feb 22, 2023

High Level Overview of Change

#4247 made some unnecessary breaking changes to the API for the validation subscription stream. This PR reverts those changes.

Type of Change

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

@ckniffen
Copy link
Collaborator

I would prefer keeping the new fields and the old. We can then transition over to the new fields after it passes and deprecate the old fields.

@@ -412,24 +412,10 @@ class Subscribe_test : public beast::unit_test::suite
return false;

bool xrpFees = env.closed()->rules().enabled(featureXRPFees);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This variable is no longer used.

@ckniffen
Copy link
Collaborator

It would also be nice to see these new fields added to server_info. I erroneously thought they were in my initial analysis.

@ximinez
Copy link
Collaborator Author

ximinez commented Feb 23, 2023

I would prefer keeping the new fields and the old. We can then transition over to the new fields after it passes and deprecate the old fields.

I disagree. I don't think adding the new fields was a good idea in the first place. I honestly can't remember why I did it. The values should be able to be changed in place without any negative effects (AFAIK).

* Review feedback from @ckniffen. Remove unused variable.
* upstream/develop:
  Fix Conan version constraint in workflows (XRPLF#4430)
  Refactor getTrustedForLedger() (XRPLF#4424)
  README: Update "Build from source" section (XRPLF#4426)
@ckniffen
Copy link
Collaborator

I thought they were changed to normalize all fees to drops. No worries though.

@intelliot intelliot assigned ximinez and unassigned thejohnfreeman and drlongle Feb 24, 2023
@intelliot
Copy link
Collaborator

intelliot commented Feb 24, 2023

@ximinez at your convenience, please confirm that this is ready to merge. You can put the Passed label on it.

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 24, 2023
@intelliot intelliot merged commit caf4827 into XRPLF:develop Feb 24, 2023
@ximinez ximinez deleted the xrpfees-api branch February 25, 2023 23:46
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 28, 2023
* upstream/develop:
  Undo API changes introduced in XRPFees: (XRPLF#4429)
  Remove recipe for RocksDB and add recipe for Snappy (XRPLF#4431)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 28, 2023
* upstream/develop:
  Undo API changes introduced in XRPFees: (XRPLF#4429)
  Remove recipe for RocksDB and add recipe for Snappy (XRPLF#4431)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 28, 2023
* upstream/develop:
  Undo API changes introduced in XRPFees: (XRPLF#4429)
  Remove recipe for RocksDB and add recipe for Snappy (XRPLF#4431)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 28, 2023
* upstream/develop:
  Undo API changes introduced in XRPFees: (XRPLF#4429)
  Remove recipe for RocksDB and add recipe for Snappy (XRPLF#4431)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 28, 2023
* upstream/develop:
  Undo API changes introduced in XRPFees: (XRPLF#4429)
  Remove recipe for RocksDB and add recipe for Snappy (XRPLF#4431)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 28, 2023
…ctionality

* upstream/develop:
  Undo API changes introduced in XRPFees: (XRPLF#4429)
  Remove recipe for RocksDB and add recipe for Snappy (XRPLF#4431)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 28, 2023
…tpage

* upstream/develop:
  Undo API changes introduced in XRPFees: (XRPLF#4429)
  Remove recipe for RocksDB and add recipe for Snappy (XRPLF#4431)
dangell7 pushed a commit to Transia-RnD/rippled that referenced this pull request Mar 5, 2023
@intelliot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change Bug Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Make XRPFees a non-breaking API change
6 participants