3.0.0: Remove `IntDecoding` as a REST option & support native bigint types in models #852

jasonpaulos · 2024-01-10T19:08:19Z

Fixes #848 (that issue contains a good description of this work)

jannotti

I don't think I understand how everything fits together, but here are some comments before I'm ooo.

jannotti · 2024-02-15T15:49:37Z

examples/accounts.ts


  console.log(
-    `Account Info: ${JSON.stringify(acctInfo)} Auth Addr: ${
+    `Account Info: ${algosdk.stringifyJSON(acctInfo)} Auth Addr: ${


My js is very rusty, but isn't it the case that (in browser anyway) console.log is clever enough to give you a little UI to look at an object if you just log it separately? This is, isn't console.log("Account Info", acctInfo) going to be more helpful?

My experience is that, yes, console.log("Account Info", acctInfo) would expand acctInfo for you in browsers, and it's usually a drop down that you can interact with to see each field. However, if you're not on a browser, it's not a great experience as often you'll just get back a string like [object, object].

src/client/kmd.ts

src/client/v2/algod/models/types.ts

jannotti · 2024-02-15T16:12:19Z

src/client/v2/algod/models/types.ts

+    this.extraProgramPages =
+      typeof extraProgramPages === 'undefined'
+        ? undefined
+        : ensureSafeInteger(extraProgramPages);


Why wouldn't we want to coerce to 0 instead of undefined? The question probably comes from me not having enough context to know where this type is used.

IMO 0 would be preferable here. Since this is a generated file, all we know is that the REST API lists that field as optional. At this level we have no way of knowing if that's the field is omitempty, or if it's truly optional

src/client/v2/algod/models/types.ts

jannotti · 2024-02-15T16:17:13Z

src/client/v2/algod/models/types.ts

+    this.budgetAdded =
+      typeof budgetAdded === 'undefined'
+        ? undefined
+        : ensureSafeInteger(budgetAdded);


This is a repeat of a previous question (why not coerce to 0). But more broadly, could ensureSafeInteger accept undefined and return 0?

ensureSafeInteger(undefined) will error. If you wanted it to coerce undefined to 0, you could do so with ensureSafeInteger(x ?? 0) (the ?? being the null coalescing operator, which is relatively new). The Transaction constructor uses this pattern a lot

src/client/v2/algod/models/types.ts

jannotti · 2024-02-15T16:22:38Z

src/client/v2/algod/models/types.ts

+    this.apps =
+      typeof apps === 'undefined' ? undefined : apps.map(ensureBigInt);


I'm repeating myself somewhat as I think I'm understanding the issue better. I think we ought to be coercing all of the "omitEmpty" fields, to their "empty" values. So lists become [], integers become 0 or 0n. I've only been commenting on the places you've had to edit because of the bignum change, but this seems like something we ought to do for all of these types before handing them to the user. They shouldn't have to understand omitEmpty.

I agree with this a lot. I'll think more about how this can be addresses

It feels like it can be another PR, but would be very nice to do for v3.

jannotti · 2024-02-15T16:26:54Z

src/client/v2/algod/setSyncRound.ts


  async do(headers = {}) {
-    const res = await this.c.post(this.path(), null, undefined, headers);
+    const res = await this.c.post({


Similar to the KmdClient.post suggestion but deeper down, should we change HTTPClient to make these call sites simpler, since they are the common case. Seems like parseBody and jsonOptions should default to these, so you can go back to simple call sites for most api calls (or add a new method like apiPost)

I'm also unhappy with the HTTPClient methods at the moment, though I didn't know whether I should address them in this PR.

My inclination is to simplify those methods so that they just return the raw Uint8Array bytes, and perhaps introduce new methods which would return parsed JSON.

So it would be something like HTTPClient.get and HTTPClient.getJSON -- still thinking through this

jannotti · 2024-02-15T16:31:11Z

src/types/intDecoding.ts

   * Number.MAX_SAFE_INTEGER will lose precision.
   */
-  DEFAULT = 'default',
+  UNSAFE = 'unsafe',


github won't let me comment in the right place. Are we still using MIXED? I get the impression that you don't need it because you can use bigint, then your coerce routines will fix things up.

I don't think we use currently using MIXED anywhere, but it's still a potentially useful option to have, especially since we now expose the JSON parsing to users

I don't think I followed why we should expose the json parsing to users. Is the idea that they may not want our models, but want to use the sdk to hit an endpoint then parse it (differently) themselves?

Right now it's expected that many of our type representations can be encoded into JSON and msgpack and back, and not just by our code -- we expect the user to be able to perform these encoding operations if they want to persist things to disk, for instance. Many of our types have a get_object_for_encoding method which returns an object ready to be serialized into one of those formats. Unfortunately bigints aren't supported by the standard JSON encoder, so without exposing the additional utilities, there wouldn't be an easy way to encode these newer objects in v3.

Technically this was also a problem in v2, but due to how types stored internal representations (they'd store exactly what they got most of the time), it was only a problem if you created a type with a bigint.

gmalouf

I'd like to see discussion to JJ's questions around the right default types for not set/not existing (nils vs zero values). Outside of that, this largely aligns with your description of the issue.

gmalouf · 2024-02-15T16:05:42Z

src/dryrun.ts

  });
 }

-interface StackValueResponse {


This file's changes feel somewhat unrelated to the stated goal of the PR, can you walk me through the intentions here?

Good question. This code broke because algod responses now contain bigints for certain fields. I could have just changed those fields, but I noticed a lot of the definitions here were redundant and no longer necessary, since we have fully typed algod responses.

src/client/v2/algod/models/types.ts

jasonpaulos · 2024-02-15T16:47:11Z

src/client/v2/algod/models/types.ts

I meant to comment this earlier, but this file is generated by https://github.com/algorand/generator. See algorand/generator#72 for the changes to the template

jasonpaulos · 2024-02-15T16:47:27Z

src/client/v2/indexer/models/types.ts

This is also generated in the same way: algorand/generator#72

algorandskiy

LGTM but not sure about v2 client backward compatibility, please advise.

src/client/client.ts

src/client/v2/indexer/models/types.ts

gmalouf

I'm good at this point.

jasonpaulos changed the title ~~Remove IntDecoding as a REST option & support native bigint types in models~~ 3.0.0 Remove IntDecoding as a REST option & support native bigint types in models Jan 10, 2024

jasonpaulos changed the title ~~3.0.0 Remove IntDecoding as a REST option & support native bigint types in models~~ 3.0.0: Remove IntDecoding as a REST option & support native bigint types in models Jan 10, 2024

jasonpaulos mentioned this pull request Jan 10, 2024

3.0.0: Remove API response int decoding option, support BigInt everywhere #816

Closed

8 tasks

jasonpaulos force-pushed the remove-intdecoding branch from e6e7351 to 9839a45 Compare February 13, 2024 18:34

jasonpaulos added 2 commits February 13, 2024 16:00

Remove intdecoding configuration from REST clients & requests

aabce7d

Fix empty address checking wrong variable

31c6054

jasonpaulos force-pushed the remove-intdecoding branch from f42bdfe to 31c6054 Compare February 13, 2024 21:00

jasonpaulos added 2 commits February 14, 2024 14:12

Make HTTPClient methods use param objects

3ba2ff6

Update kmd and migration guide

dfae46c

jasonpaulos marked this pull request as ready for review February 14, 2024 22:22

Migration guide tweaks

9e2acc4

jasonpaulos mentioned this pull request Feb 15, 2024

JS v3: Native bigint support for select fields algorand/generator#72

Merged

jasonpaulos requested review from algorandskiy, gmalouf and jannotti February 15, 2024 15:33

jannotti reviewed Feb 15, 2024

View reviewed changes

gmalouf reviewed Feb 15, 2024

View reviewed changes

jasonpaulos commented Feb 15, 2024

View reviewed changes

src/client/v2/indexer/models/types.ts

Copy link

Contributor Author

jasonpaulos Feb 15, 2024

Choose a reason for hiding this comment

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

This is also generated in the same way: algorand/generator#72

Helper methods for kmd

80a0c35

algorandskiy reviewed Feb 16, 2024

View reviewed changes

src/client/client.ts Show resolved Hide resolved

src/client/v2/indexer/models/types.ts Show resolved Hide resolved

gmalouf approved these changes Feb 16, 2024

View reviewed changes

algorandskiy approved these changes Feb 16, 2024

View reviewed changes

jasonpaulos merged commit 96c2295 into 3.0.0 Feb 16, 2024

jasonpaulos deleted the remove-intdecoding branch February 16, 2024 20:36

jasonpaulos mentioned this pull request Aug 21, 2024

FOR REVIEW ONLY: js-algorand-sdk v3.0.0-beta.1 #891

Closed

onetechnical mentioned this pull request Sep 12, 2024

FOR REVIEW ONLY: js-algorand-sdk v3.0.0 #901

Merged

		this.apps =
		typeof apps === 'undefined' ? undefined : apps.map(ensureBigInt);

3.0.0: Remove IntDecoding as a REST option & support native bigint types in models #852

3.0.0: Remove IntDecoding as a REST option & support native bigint types in models #852

Uh oh!

Conversation

jasonpaulos commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jannotti Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jannotti Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmalouf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

algorandskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gmalouf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

3.0.0: Remove `IntDecoding` as a REST option & support native bigint types in models #852

3.0.0: Remove `IntDecoding` as a REST option & support native bigint types in models #852

jasonpaulos commented Jan 10, 2024 •

edited

Loading

jannotti Feb 15, 2024 •

edited

Loading

jannotti Feb 15, 2024 •

edited

Loading