-
Notifications
You must be signed in to change notification settings - Fork 206
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
Conversation
@jasonpaulos
|
@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. |
If you want me to I can remove the bigint changes in this PR. I'm not super opinionated on having 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. |
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.
I still think our BlockHeader
needs to be updated to be correct, but that can be worked out later. Let's get this merged
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.