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.

Sign up for GitHub

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

Jump to bottom

refactor: simplify TxFormats common fields logic #4637

Merged
merged 4 commits into from
Sep 8, 2023
Merged

refactor: simplify TxFormats common fields logic #4637

merged 4 commits into from
Sep 8, 2023

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Jul 31, 2023

High Level Overview of Change

This PR proposes a minor refactor to ripple/protocol/TxFormats.cpp, renaming commonFields to pseudoCommonFields (since it is the common fields that all pseudo-transactions need) and adding a new static variable, commonFields, which represents all the common fields that non-pseudo transactions need (essentially everything that pseudoCommonFields contains, plus sfTicketSequence).

Context of Change

This makes it harder to accidentally leave out sfTicketSequence in a new transaction.

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 Updates
  • Release

@seelabs seelabs requested review from seelabs and HowardHinnant July 31, 2023 20:55
seelabs
seelabs reviewed Jul 31, 2023
View reviewed changes
src/ripple/protocol/impl/TxFormats.cpp Outdated Show resolved Hide resolved
src/ripple/protocol/impl/TxFormats.cpp Outdated Show resolved Hide resolved
seelabs
seelabs approved these changes Aug 1, 2023
View reviewed changes
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

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

👍 Nice refactor.

@nbougalis
Copy link
Contributor

I’d simply move the ticket field out of the individual transaction formats and into the common fields and then add a manual check in Transactor.cpp to ensure that transactions that aren’t supposed to have this field (i.e. system generated pseudo-transactions) don’t accidentally include it.

@intelliot
Copy link
Collaborator

I’d simply move the ticket field out of the individual transaction formats and into the common fields and then add a manual check in Transactor.cpp to ensure that transactions that aren’t supposed to have this field (i.e. system generated pseudo-transactions) don’t accidentally include it.

Given this PR has two approvals, I think we can put this in for now, and consider the above suggestion in a separate/later PR.

@intelliot
Copy link
Collaborator

@mvadari - at your convenience, you can resolve the conflict so that this PR would be able to merge cleanly on develop

@intelliot intelliot added this to the 1.13 milestone Sep 1, 2023
@mvadari mvadari force-pushed the ticket-sequence-simplify branch from 199bbad to 97d89af Compare September 1, 2023 11:02
@intelliot
Copy link
Collaborator

(Since this PR was force-pushed, let's get 1 re-review from @HowardHinnant , at his convenience)

@intelliot intelliot added the Tech Debt Non-urgent improvements label Sep 1, 2023
@mvadari
Copy link
Collaborator Author

mvadari commented Sep 1, 2023

(Since this PR was force-pushed, let's get 1 re-review from @HowardHinnant , at his convenience)

Sorry, was I supposed to merge it? I thought you preferred rebases + force pushes.

@intelliot
Copy link
Collaborator

FWIW, I prefer merge commits. It's the way git is designed to be used.

@intelliot intelliot merged commit 31c8281 into XRPLF:develop Sep 8, 2023
@mDuo13 mDuo13 mentioned this pull request Sep 18, 2023
Closed
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
@mvadari @ckeshava
refactor: simplify TxFormats common fields logic (XRPLF#4637)
19eb419 
Minor refactor to `TxFormats.cpp`:
- Rename `commonFields` to `pseudoCommonFields` (since it is the common fields
  that all pseudo-transactions need)
- Add a new static variable, `commonFields`, which represents all the common
  fields that non-pseudo transactions need (essentially everything that
  `pseudoCommonFields` contains, plus `sfTicketSequence`)

This makes it harder to accidentally leave out `sfTicketSequence` in a new
transaction.
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
@mvadari @ckeshava
refactor: simplify TxFormats common fields logic (XRPLF#4637)
9d17d4c 
Minor refactor to `TxFormats.cpp`:
- Rename `commonFields` to `pseudoCommonFields` (since it is the common fields
  that all pseudo-transactions need)
- Add a new static variable, `commonFields`, which represents all the common
  fields that non-pseudo transactions need (essentially everything that
  `pseudoCommonFields` contains, plus `sfTicketSequence`)

This makes it harder to accidentally leave out `sfTicketSequence` in a new
transaction.
intelliot pushed a commit that referenced this pull request Oct 5, 2023
@mDuo13
refactor: reunify transaction common fields: (#4715)
4e84ad6 
Make transactions and pseudo-transactions share the same commonFields
again. This regularizes the code in a nice way.

While this technically allows pseudo-transactions to have a
TicketSequence field, pseudo-transactions are only ever constructed by
code paths that don't add such a field, so this is not a transaction
processing change. It may be possible to add a separate check to ensure
TicketSequence (and other fields that don't make sense on
pseudo-transactions) are never added to pseudo-transactions, but that
should not be necessary. (TicketSequence is not the only common field
that can not and does not appear in pseudo-transactions.) Note:
TicketSequence is already documented as a common field.

Related: #4637

Fix #4714
florent-uzio pushed a commit to florent-uzio/rippled that referenced this pull request Oct 6, 2023
@mDuo13 @florent-uzio
refactor: reunify transaction common fields: (XRPLF#4715)
f55fa1d 
Make transactions and pseudo-transactions share the same commonFields
again. This regularizes the code in a nice way.

While this technically allows pseudo-transactions to have a
TicketSequence field, pseudo-transactions are only ever constructed by
code paths that don't add such a field, so this is not a transaction
processing change. It may be possible to add a separate check to ensure
TicketSequence (and other fields that don't make sense on
pseudo-transactions) are never added to pseudo-transactions, but that
should not be necessary. (TicketSequence is not the only common field
that can not and does not appear in pseudo-transactions.) Note:
TicketSequence is already documented as a common field.

Related: XRPLF#4637

Fix XRPLF#4714
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
@mvadari @sophiax851
refactor: simplify TxFormats common fields logic (XRPLF#4637)
937354b 
Minor refactor to `TxFormats.cpp`:
- Rename `commonFields` to `pseudoCommonFields` (since it is the common fields
  that all pseudo-transactions need)
- Add a new static variable, `commonFields`, which represents all the common
  fields that non-pseudo transactions need (essentially everything that
  `pseudoCommonFields` contains, plus `sfTicketSequence`)

This makes it harder to accidentally leave out `sfTicketSequence` in a new
transaction.
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
@mDuo13 @sophiax851
refactor: reunify transaction common fields: (XRPLF#4715)
64fbbf5 
Make transactions and pseudo-transactions share the same commonFields
again. This regularizes the code in a nice way.

While this technically allows pseudo-transactions to have a
TicketSequence field, pseudo-transactions are only ever constructed by
code paths that don't add such a field, so this is not a transaction
processing change. It may be possible to add a separate check to ensure
TicketSequence (and other fields that don't make sense on
pseudo-transactions) are never added to pseudo-transactions, but that
should not be necessary. (TicketSequence is not the only common field
that can not and does not appear in pseudo-transactions.) Note:
TicketSequence is already documented as a common field.

Related: XRPLF#4637

Fix XRPLF#4714
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HowardHinnant HowardHinnant HowardHinnant approved these changes

@seelabs seelabs seelabs approved these changes

Assignees
No one assigned
Labels
Tech Debt Non-urgent improvements
Projects
Core Ledger
Archived in project
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mvadari @nbougalis @intelliot @seelabs @HowardHinnant