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

Update sign-and-submit command to include accepted, broadcast, queued and other fields #4864

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

ckeshava
Copy link
Collaborator

@ckeshava ckeshava commented Jan 3, 2024

High Level Overview of Change

Fix #3284

This PR purports to make the response of the submit command identical -- irrespective of whether a binary blob or JSON is provided as input to the command.

Context of Change

The linked issue provides a description of the context of the change, actual and expected behaviors.

Type of Change

  • 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 update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

This change adds accepted, queued, broadcast and other fields that were originally included in PR #3125

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Before / After

With this change, the cited example in the bug page (payment transaction from the genesis account) returns the expected behavior i.e. accepted, broadcast and other fields are returned in the output.

@ckeshava
Copy link
Collaborator Author

ckeshava commented Jan 3, 2024

@mDuo13 @ximinez @Bronek can you please review this PR? thanks!

@ckeshava ckeshava changed the title Update sign-and-submit command to include all fields Update sign-and-submit command to include accepted, broadcast, queued and other fields Jan 3, 2024
@@ -6216,9 +6216,46 @@ class RPCCall_test : public beast::unit_test::suite
}
}

void
testSubmitResponseFields()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do I need to modify this unit test to incorporate apiVersion ? Since I'm adding new fields, it shouldn't break backwards compatibility even if an older version of rippled recieves the new output

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct, this should not be a breaking change and therefore should not be on the next apiVersion.

@intelliot intelliot added this to the 2.1.0 (Feb 2024) milestone Jan 8, 2024
@intelliot
Copy link
Collaborator

@intelliot intelliot added the Perf impact not expected Change is not expected to improve nor harm performance. label Jan 12, 2024
@intelliot
Copy link
Collaborator

@ckeshava what's the status of this, from your perspective?

@ckeshava
Copy link
Collaborator Author

As it stands, I don't have anything else to add to the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Perf impact not expected Change is not expected to improve nor harm performance.
Projects
Status: 👀 Needs review
Development

Successfully merging this pull request may close these issues.

Augmented submit fields should be included in sign-and-submit mode (Version: 1.5.0-b6)
2 participants