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

feat: add From<Transaction> and From<&Transaction> #370

Merged
merged 1 commit into from
May 26, 2024

Conversation

willemneal
Copy link
Member

What

Tools need to always expect TransactionEvleope at the headings. This makes it easy to go in one direction.

Why

[TODO: Why this change is being made. Include any context required to understand the why.]

Known limitations

[TODO or N/A]

@willemneal
Copy link
Member Author

@leighmcculloch should we also include a TryFrom<TransactionEnvelope>? If so would we need upstream changes to add an error case?

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I left a few comments inline.

I think we can add this conversion because it is common to build transactions and to want an envelope. It's also reasonable to do the same for FeeBumpTransaction too, so I think that could be added.

should we also include a TryFrom? If so would we need upstream changes to add an error case?

I don't think we should add reverse conversions. The reverse conversion would discard data which could result in bugs for round trip operations, which are often the types of operations we perform with transactions. And the match keyword is convenient enough and ensures devs handle all cases or be very explicitly not handling some.

For the existing tests, I'd leave them building TEs, as there's little succinctness introduced at the cost of the .into() obfuscating.

Because the comments inline are small, I'll update them and merge the change.

src/next/transaction_conversions.rs Outdated Show resolved Hide resolved
src/curr/transaction_conversions.rs Outdated Show resolved Hide resolved
src/curr/mod.rs Show resolved Hide resolved
@willemneal willemneal force-pushed the feat/from_transaction branch 2 times, most recently from d023e37 to 3bf31d7 Compare May 22, 2024 18:55
@willemneal willemneal force-pushed the feat/from_transaction branch from 3bf31d7 to a83dff1 Compare May 22, 2024 18:57
@willemneal
Copy link
Member Author

@leighmcculloch I removed the edits to the previous tests.

@leighmcculloch leighmcculloch added this pull request to the merge queue May 26, 2024
Merged via the queue into stellar:main with commit d368909 May 26, 2024
10 checks passed
@willemneal willemneal deleted the feat/from_transaction branch May 27, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants