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

Redefine TicketSequence as a common field #4714

Closed
mDuo13 opened this issue Sep 18, 2023 · 1 comment · Fixed by #4715
Closed

Redefine TicketSequence as a common field #4714

mDuo13 opened this issue Sep 18, 2023 · 1 comment · Fixed by #4715
Labels
Low Priority Tech Debt Non-urgent improvements

Comments

@mDuo13
Copy link
Collaborator

mDuo13 commented Sep 18, 2023

Summary

Relevant source file: TxFormats.cpp

The TicketSequence field of transactions is implemented as a transaction-specific field that is separately added on every type of transaction, but not on pseudo-transactions. This is unnecessarily complexity and may lead to errors later. It should be made a common field instead.

Motivation

It's strange to have one "common field" that is sort-of-technically not one. I believe the original intent was to make sure that pseudo-transactions can never have a TicketSequence, but there's no need to make it not a common field for that to happen; plenty of other common fields also cannot and don't appear in pseudo-transactions.

Meanwhile, having it be not "technically" a common field means that each new transaction type has to have it added. It would be an easy mistake to add a new transaction type and forget to add it. This special case complicates code analysis and documentation. (Currently TicketSequence is documented as if it was a common field, which means people comparing the code to the docs may be confused why it's not listed with the other common fields, and people looking at the transactions' source code may similarly wonder why one of the "specific" fields for that transaction isn't listed as a specific field for that transaction.

Solution

Redefine TicketSequence as a common field of all transaction types.

I am not 100% certain of this, but I believe that with the way transactions' canonical format is defined, this would not change the actual format for any type of transaction that can be submitted; as for pseudo-transactions, it would not meaningfully impact them because those can only be created in the server code that does not use TicketSequence. Therefore, it would not be a transaction processing change and would not require an amendment.

That said, this change would change the order in which fields are added to transaction types in TxFormats.cpp. If for some reason the order matters, then it would be a transaction processing change that needs to be on an amendment (and may not be worth doing).

This can be tested by serializing a transaction with TicketSequence to binary on a server with the code change and one without it; if the two are identical, then it is not a transaction processing change. (Maybe, to be sure, it should be on a transaction type that defines a field after TicketSequence and should use that field. That would be an AccountSet with NFTokenMinter or an AMMDeposit with TradingFee.)

@mDuo13 mDuo13 added Tech Debt Non-urgent improvements Low Priority labels Sep 18, 2023
@mDuo13
Copy link
Collaborator Author

mDuo13 commented Sep 18, 2023

EDIT: I just discovered that this has already been (sorta) done in the develop branch by #4637. (I was looking at master.) I believe we can safely remove the macro to separate "common fields" for pseudo- and non-pseudo transactions.

mDuo13 added a commit to mDuo13/rippled that referenced this issue Sep 18, 2023
Makes transactions and pseudo-transactions share the same commonFields
again. This change technically allows pseudo-transactions to have a
TicketSequence field, but those transactions are only ever constructed
by code paths that don't add such a field, so this is not actually a
transaction processing change.

We could optionally add a separate check to make sure this and other
fields that don't make sense on pseudo-transactions are never added to
them, but I don't think that's necessary.

Fixes XRPLF#4714
intelliot pushed a commit that referenced this issue Oct 5, 2023
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 issue Oct 6, 2023
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 issue Jun 12, 2024
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
Labels
Low Priority Tech Debt Non-urgent improvements
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant