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

Add ID field to Transaction and V2Transaction JSON encoding #231

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

n8maninger
Copy link
Member

@n8maninger n8maninger commented Nov 13, 2024

This adds the calculated ID for the transaction to the JSON marshaling of a transaction. This is primarily a convenience for API consumers that may or may not have the ability to calculate it easily. I believe siad added a calculate endpoint, but it's even more annoying to have to call 2 routes imo.

I decided not to override unmarshal and discard the ID for similar convenience, but it could be desirable to validate the deserialized transaction data against the provided ID. However, that would complicate broadcasting a transaction.

Most recent context, but this comes up quite a lot with integrators: https://discord.com/channels/809849352516141067/809858207064653894/1306241130408443915.

@lukechampine
Copy link
Member

Intriguing. In the past we've wrapped Transaction in API-specific types that add an ID field, but it's not hard to see the disadvantages of that approach.

Introducing "synthetic" fields like this to a marshalled object does make me a little uneasy; I don't like the fact that the encoding does not map 1-to-1 to the Go struct, and I don't like the unmarshal asymmetry either. But the benefits might outweigh the costs in this case.

types/types.go Outdated Show resolved Hide resolved
types/types.go Outdated Show resolved Hide resolved
@n8maninger
Copy link
Member Author

n8maninger commented Nov 13, 2024

Intriguing. In the past we've wrapped Transaction in API-specific types that add an ID field, but it's not hard to see the disadvantages of that approach.

Yeah, having to wrap all the API responses with a distinct transaction type is less than ideal. Not a bad way to solve the problem, though. I definitely lean more towards enforced global convenience for JSON consumers over purity in this case. It does get kind of "dicey" with this route though: where do we draw the line? Siacoin Siafund UTXOs and File Contracts also have IDs that are not easily exposed.

Synthetic fields may be gross, but I find solace in the knowledge that it shares that grotesquerie with V2FileContractResolution and SpendPolicy.

@lukechampine
Copy link
Member

where do we draw the line? Siacoin Siafund UTXOs and File Contracts also have IDs that are not easily exposed.

hmm, yeah. They're also more problematic to compute: you need access to the Transaction containing them. We could make (Transaction).MarshalJSON really special and override all of its children to include their ID, but that would be really ugly (as well as really slow, at least for v1 transactions -- which require you to rehash the entire transaction for every ID). In practice I imagine it's less common for consumers to care about those IDs anyway, though.

Block.ID(), though... that might be worth adding.

Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

Lgtm. We did a similar thing in renterd for fields that we just don't need internally since we compute them on-the-fly but having them in the API response is very nice for API consumers.

@n8maninger n8maninger merged commit 0b6d8d1 into master Nov 14, 2024
9 checks passed
@n8maninger n8maninger deleted the nate/add-txn-id branch November 14, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants