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

Align transaction fields to transaction reference spec #804

Merged
merged 9 commits into from
Aug 11, 2023

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Aug 1, 2023

This PR contains breaking changes in transaction fields that align more closely to the transaction reference spec.

Changes:

  • from -> sender
  • to -> receiver
  • firstRound -> firstValid
  • lastRound -> lastValid
  • freezeState -> assetFrozen
  • assetRevocationTarget -> assetSender
  • reKeyTo -> rekeyTo

#144

@algochoi algochoi changed the title Align transaction fields to reference spec Align transaction fields to transaction reference spec Aug 1, 2023
@algochoi
Copy link
Contributor Author

algochoi commented Aug 1, 2023

@algoanne We have two places where we use application-index in our APIs: https://developer.algorand.org/docs/rest-apis/algod/#pendingtransactionresponse but the most recent ones refer to it as application-id. Do we want to make this change here for app/assets?

@algoanne
Copy link

algoanne commented Aug 2, 2023

@algochoi it seems ID is more common. Disappointing that we're not fully consistent at the API level. I really don't feel strongly one way or the other, they both make me sad. I leave it to you.

@algochoi
Copy link
Contributor Author

algochoi commented Aug 7, 2023

They also make me sad, and I would like to consolidate everything to the newer convention of IDs.

However, we also have places in the API that use Index such as Assets.
Because of these inconsistencies that will also affect how the SDK types will be generated, e.g. here, I'm inclined to keep everything as-is to be consistent with the API spec. I think changing these index-related conventions should be following a breaking change in the API spec as well.

@algochoi algochoi marked this pull request as ready for review August 7, 2023 18:31
@jasonpaulos
Copy link
Contributor

They also make me sad, and I would like to consolidate everything to the newer convention of IDs.

However, we also have places in the API that use Index such as Assets. Because of these inconsistencies that will also affect how the SDK types will be generated, e.g. here, I'm inclined to keep everything as-is to be consistent with the API spec. I think changing these index-related conventions should be following a breaking change in the API spec as well.

I have a hard time believing that we'll want to break the REST APIs anytime soon with an index => ID change, so it's worth thinking about what we can do short of that solving the problem.

We do have some cases where the REST API uses ID instead of index, for example here.

One option we could consider is modifying the generator repo to rename certain fields from index => ID. Then we could use ID everywhere, including in the SDK defined classes.

@algochoi
Copy link
Contributor Author

algochoi commented Aug 9, 2023

I have a hard time believing that we'll want to break the REST APIs anytime soon with an index => ID change, so it's worth thinking about what we can do short of that solving the problem.

I agree as well -- I wanted to skirt around this issue for now because it seemed like the most onerous change (requires some custom overrides in generator, and then it may affect the three SDKs we generate for in order to keep field names consistent with each other, plus any overhead that these changes might have in the tests/examples/dev docs). On the other hand, the current changes are pretty isolated to the JS SDK and are easier wins and has had enough notice to dev-rel.

@algoanne
Copy link

algoanne commented Aug 9, 2023

I agree as well -- I wanted to skirt around this issue for now because it seemed like the most onerous change (requires some custom overrides in generator, and then it may affect the three SDKs we generate for in order to keep field names consistent with each other, plus any overhead that these changes might have in the tests/examples/dev docs). On the other hand, the current changes are pretty isolated to the JS SDK and are easier wins and has had enough notice to dev-rel.

I think I support breaking out the index -> ID changes into a separate PR, since it potentially has broader scope (across all the SDKs). Although we have not been explicitly planning a 3.0 for the other SDKs, it's certainly on the table. It would be valuable to bring this same consistency to all the SDKs. We can discuss.
Do the other SDKs have consistent naming with the API in the other fields, e.g. lastValid etc. ?

@algochoi
Copy link
Contributor Author

algochoi commented Aug 9, 2023

Do the other SDKs have consistent naming with the API in the other fields, e.g. lastValid etc. ?

I have only checked the obvious ones listed above (sender, lastValid), but yes. The go-sdk closely follows go-algorand conventions, Java is pretty similar, and Python has some minor variations in naming but still makes sense to me.

The generated SDKs (Go, Java, JS) are also consistent with the API spec, which itself has some inconsistent conventions like using index for Assets but asset-id for AssetHoldings. So I think changing these would probably warrant a change in all the SDKs. I haven't thought about potential downstream impacts here (e.g. Swift, C# SDKs...).

@winder
Copy link
Contributor

winder commented Aug 10, 2023

I'm fine with ignoring the ID/Index issue, but we also have a lot of flexibility in the generator with applying things to a single SDK

Copy link
Contributor

@bbroder-algo bbroder-algo left a comment

Choose a reason for hiding this comment

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

s-tier

@bbroder-algo bbroder-algo merged commit c079c61 into 3.0.0 Aug 11, 2023
2 checks passed
@algochoi algochoi deleted the breaking-tx-fields branch August 11, 2023 15:46
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.

6 participants