Skip to content

chore(proto, core)!: remove action suffix from all action types#1630

Merged
SuperFluffy merged 11 commits intomainfrom
ENG-907/remove-action-suffixes
Oct 8, 2024
Merged

chore(proto, core)!: remove action suffix from all action types#1630
SuperFluffy merged 11 commits intomainfrom
ENG-907/remove-action-suffixes

Conversation

@SuperFluffy
Copy link
Contributor

@SuperFluffy SuperFluffy commented Oct 3, 2024

Summary

This patch removes the *Action suffix from all protobuf on-wire and domain types that represent sequencer actions.

Background

The *Action suffix is redundant because at no point in the last 12 months was there a risk of confusing action types with non-action types that have a similar name (if there are any even).

Furthermore, we follow Rust naming conventions for types. Since we defined domain types in a module called action, clippy's module_name_repetition warning triggered on the names, requiring allow attributes to quiet the warnings and to stay in sync with the on-wire protobuf types.

Changes

  • Remove the Action suffix from all action protobuf and domain types
  • Rename transactions -> transaction in the former astria.protocol.transactions.v1alpha1
  • Move all action protobuf types to a new file action.proto (similar to action.rs), but keeping the same astria.protocol.transaction.v1alpha1 protobuf package (because actions don't exist outside of transactions).
  • Regenerate rust protobuf decoding/encoding code
  • Rename enum FeeChange to enum FeeChangeKind because the former FeeChangeAction now clashes with the enum.

Testing

Only type names were changed. The code still compiles and all tests run. No business logic was changed.

Breaking Changelist

  • This is not a network breaking change because domain type names (or protobuf names) do not manifest in business logic outside of tracing and logging.
  • This is a breaking change for consumers of our protobuf API if they explicitly referred to action names.

Related Issues

Closes #1629

@SuperFluffy SuperFluffy requested a review from joroshiba October 3, 2024 18:57
@github-actions github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate composer pertaining to composer labels Oct 3, 2024
@SuperFluffy SuperFluffy force-pushed the ENG-907/remove-action-suffixes branch from 6665ccd to 7287613 Compare October 3, 2024 18:59
@SuperFluffy SuperFluffy marked this pull request as ready for review October 3, 2024 19:00
@SuperFluffy SuperFluffy requested review from a team as code owners October 3, 2024 19:00
joroshiba
joroshiba previously approved these changes Oct 3, 2024
Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

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

Protobuf changes LGTM

@joroshiba joroshiba dismissed their stale review October 3, 2024 19:54

premature

package astria.protocol.transaction.v1alpha1;

import "astria/primitive/v1/types.proto";
import "astria/protocol/transaction/v1alpha1/actions.proto";
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe you need to import these since they are in the same package.

Copy link
Contributor Author

@SuperFluffy SuperFluffy Oct 4, 2024

Choose a reason for hiding this comment

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

Unfortunately you do (assumed the same and tried without at first). Proof:

import "astria/primitive/v1/types.proto";
// import "astria/protocol/transaction/v1alpha1/actions.proto";
import "astria_vendored/penumbra/core/component/ibc/v1/ibc.proto";
import "astria_vendored/tendermint/abci/types.proto";
import "google/protobuf/any.proto";
proto/protocolapis/astria/protocol/transaction/v1alpha1/transaction.proto:25:12:field astria.protocol.transaction.v1alpha1.UnsignedTransaction.actions: unknown type Action

@SuperFluffy SuperFluffy requested a review from joroshiba October 4, 2024 13:09
move action protos to new file, rename types.proto to transaction.proto

actions.proto -> action.proto
@SuperFluffy SuperFluffy force-pushed the ENG-907/remove-action-suffixes branch from d901258 to a7a765b Compare October 7, 2024 12:22
@SuperFluffy
Copy link
Contributor Author

Squashed and rebased on top of recent main.

Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

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

Approved with note on yet another potential fee related change as follow up.

}

message FeeChangeAction {
message FeeChange {
Copy link
Member

Choose a reason for hiding this comment

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

Thinking on the semantics of the fee change given the need for the rust type change from this. Would it make more sense to have this message be something like:

enum FeeVariable {
  UNKNOWN_FEE_VARIABLE = 0;
  TRANSFER_BASE_FEE = 1;
  ...
}

message FeeChange {
  FeeVariable fee_variable = 1;
  astria.primitive.v1.Uint128 value = 2;
}

Then the rust fee variables could be related to the enum and the FeeChangeKind might be able to go away? Probably not for this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline: generally agree. This action is not nice to read and its semantics are off. But will push that to a different PR.

@SuperFluffy SuperFluffy added this pull request to the merge queue Oct 8, 2024
Merged via the queue into main with commit 3abd40a Oct 8, 2024
@SuperFluffy SuperFluffy deleted the ENG-907/remove-action-suffixes branch October 8, 2024 16:17
steezeburger added a commit that referenced this pull request Oct 11, 2024
* main:
  feat(sequencer)!: enforce block ordering by transaction group  (#1618)
  fix(cli): ensure checkTx passes before waiting for inclusion (#1636)
  chore(sequencer)!: update storage keys locations and values (ENG-898) (#1616)
  chore: cargo audit warning (#1644)
  chore(proto, core)!: remove action suffix from all action types (#1630)
  feat(sequencer)!: add limit to total amount of transactions in parked  (#1638)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

composer pertaining to composer proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove Address suffix from protobuf and domain types

2 participants