-
Notifications
You must be signed in to change notification settings - Fork 205
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
Conversation
@algoanne We have two places where we use |
@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. |
They also make me sad, and I would like to consolidate everything to the newer convention of However, we also have places in the API that use |
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. |
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. |
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 |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s-tier
This PR contains breaking changes in transaction fields that align more closely to the transaction reference spec.
Changes:
#144