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

3.0.0: Convert algod responses to typed #776

Merged
merged 58 commits into from
Aug 31, 2023

Conversation

Eric-Warehime
Copy link
Contributor

@Eric-Warehime Eric-Warehime commented May 5, 2023

Resolves #771 for the algod client.

I'm attempting to merge this to the 3.0.0 branch of the SDK which we will not be releasing until we've gained a critical mass of breaking changes that we feel justifies a breaking major version bump for this SDK.

Also updates the eslint dependencies to more recent versions, and fixes a couple of new lints which were failing--optional parameters at the end of the function signature, and promise return in the cucumber tests.

Existing tests pass--not sure if we need to be adding more tests here.

@Eric-Warehime
Copy link
Contributor Author

@jasonpaulos
Ok I think I've addressed all of the feedback except for the following:

  1. Indexer response typing--I've not included them in this PR. It's possible we should include them in this branch before releasing 3.0.0 since it would be a breaking change as well.
  2. Converting the storage format of everything to bigint instead of union types. This seems both like a breaking change to me, and also something that's more of an annoyance than anything else. Libraries like https://www.npmjs.com/package/json-bigint seem to solve this problem.

@jasonpaulos
Copy link
Contributor

@Eric-Warehime I realized changing the default int decoding is a bit of a larger problem. I've opened #816 to hopefully address the bigint issue.

If the idea behind that PR makes sense to you, this PR can revert the attempt to change int decoding in favor of that PR.

Everything else here looks good to me.

@Eric-Warehime
Copy link
Contributor Author

@jasonpaulos

If you want me to I can remove the bigint changes in this PR. I'm not super opinionated on having number | bigint unions w/ the deserialization prefering bigints versus what it looks like you've done in your PR which is to just convert all numbered responses to bigint exclusively.

I guess the caveat there is that you'll need to alter the code generator since you're modifying the types and it looks like your PR has some issues (possibly some of the same test failures I've already worked through here?).

As a final option I guess we could get this merged and follow up with a PR to change the unions to bigint--maybe that would just cause a lot of unnecessary work though.

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

I still think our BlockHeader needs to be updated to be correct, but that can be worked out later. Let's get this merged

@Eric-Warehime Eric-Warehime merged commit 6d17cd3 into algorand:3.0.0 Aug 31, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants