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

avoid circular dependency in hip-551 protobuf code #17360

Open
kimbor opened this issue Jan 13, 2025 · 0 comments · May be fixed by #17361
Open

avoid circular dependency in hip-551 protobuf code #17360

kimbor opened this issue Jan 13, 2025 · 0 comments · May be fixed by #17361
Assignees
Labels
Milestone

Comments

@kimbor
Copy link
Member

kimbor commented Jan 13, 2025

Problem

I ran into an issue with setting up the protobufs for batch txs. Transaction in transaction.proto contains TransactionBody in transaction_body.proto (in the deprecated body field). We want TransactionBody to contain AtomicBatchTransactionBody which contains Transaction. This sets up a circular reference and makes the protobuf compiler mad.

Solution

Option 1: Delete the deprecated body field from Transaction. I started down this path as part of PR #17333. Practically all of the places it was referenced were in tests, so it was a bit of work but not completely unreasonable. I think that if we’re going to delete the deprecated body field we should also delete the other deprecated fields in the Transaction message. However, I’m a bit leery of changing protobufs like this. Are there ancient transactions which depend on it? Is there a reason why these fields were marked deprecated instead of removed altogether? Update: this option will not work per this PR and discussion with the team

Option 2: Combine transation.proto, transaction_body.proto and atomic_batch.proto into a single proto file. This feels messy but should solve the circular dependency

Option 3: We could have a custom transaction.proto(called atomic_transaction.proto or something) with only bytes signedTransactionBytes field in it. And then AtomicBatchTransactionBody would refer to that instead of Transaction

Option 4: AtomicBatchTransactionBody should contain bytes instead of a Transaction.
Instead of this:

message AtomicBatchTransactionBody {
    repeated Transaction transactions = 1;
}

We’d have this:

message AtomicBatchTransactionBody {
    repeated bytes signedTransactionBytes = 1;
}
@kimbor kimbor self-assigned this Jan 13, 2025
@kimbor kimbor added this to the v0.60 milestone Jan 13, 2025
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Services Team Jan 13, 2025
@kimbor kimbor moved this from 📋 Backlog to 👷🏼‍♀️ In Progress in Services Team Jan 13, 2025
@kimbor kimbor changed the title feat: avoid circular dependency in hip-551 protobuf code avoid circular dependency in hip-551 protobuf code Jan 13, 2025
@kimbor kimbor added the HiP-551 label Jan 13, 2025
@kimbor kimbor modified the milestones: v0.60, v0.59 Jan 14, 2025
@kimbor kimbor moved this from 👷🏼‍♀️ In Progress to 👀 In Review in Services Team Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 👀 In Review
1 participant