Add a BigDecimal type that can be used in the indexer#15

JasoonS · 2024-06-06T07:28:05Z

No description provided.

JasoonS · 2024-06-06T07:32:17Z

@@ -0,0 +1,111 @@
+import { expect } from 'chai';
+// import { BigDecimal } from 'generated';/// For some reason this doesn't work?


I tried litrally everything under the sun to try get this to work, for some reason I couldn't :/

Anyway.

We should probably put this in a @envio/bigdecimal npm package or something rather. But this is working for now.

Any ideas what is going wrong here @JonoPrest or @DZakh ?

It would need to be exported from index.js and type exported from index.d.ts in the generated code if you want to import straight from generated.

JasoonS · 2024-06-06T07:32:34Z


+type EntityWithFields {
+  id: ID!
+  bigDecimal: BigDecimal!


Used in the test

@DenhamPreen - for the docs.

JasoonS · 2024-06-06T07:33:29Z

+      switch handlerContext.entityWithFields.get(testEntity1.id) {
+      | Some(entity) => Assert.equal(entity.bigDecimal->BigDecimal.toString, "123.456")
+      | None => Assert.fail("Entity should exist")
+      }
+      switch handlerContext.entityWithFields.get(testEntity2.id) {
+      | Some(entity) => Assert.equal(entity.bigDecimal->BigDecimal.toString, "654.321")
+      | None => Assert.fail("Entity should exist")
+      }


Tests that they are read and written to the DB correctly.

Also they use numeric in postgres - so they appear to work completely as expected.

DZakh

🙌

DZakh · 2024-06-06T07:44:07Z

+export class BigDecimal {
+  private value: BigNumber;
+
+  constructor(value: BigNumber) {
+    this.value = value;
+  }
+
+  static fromBigInt(value: BigInt): BigDecimal {
+    return new BigDecimal(new BigNumber(value as any));
+  }


Should we maybe write it in ReScript to have the same API for both ReScript and TS codebases?

I actually had this before:

@genType.import(("bignumber.js", "BigNumber")) type t // Constructors @new @module external _fromBigInt: Ethers.BigInt.t => t = "bignumber.js" @genType let fromBigInt = _fromBigInt @new @module external _fromFloat: float => t = "bignumber.js" @genType let fromFloat = _fromFloat @new @module external _fromInt: int => t = "bignumber.js" @genType let fromInt = _fromInt @new @module external _fromStringUnsafe: string => t = "bignumber.js" @genType let fromStringUnsafe = _fromStringUnsafe @new @module external _fromString: string => option<t> = "bignumber.js" @genType let fromString = _fromString // Methods @send external _toString: t => string = "toString" @genType let toString = _toString @send external _toFixed: t => string = "toFixed" @genType let toFixed = _toFixed let toInt = (b: t): option<int> => b->toString->Belt.Int.fromString // Arithmetic Operations @send external _plus: (t, t) => t = "plus" @genType let plus = _plus @send external _minus: (t, t) => t = "minus" @genType let minus = _minus @send external _times: (t, t) => t = "multipliedBy" @genType let times = _times @send external _div: (t, t) => t = "dividedBy" @genType let div = _div // @send external _pow: (t, int) => t = "toExponential" // @genType let pow = _pow // @send external _mod: (t, t) => t = "modulo" // @genType let mod = _mod // Comparison @send external _equals: (t, t) => bool = "isEqualTo" @genType let equals = _equals let notEquals: (t, t) => bool = (a, b) => !_equals(a, b) @send external _gt: (t, t) => bool = "isGreaterThan" @genType let gt = _gt @send external _gte: (t, t) => bool = "isGreaterThanOrEqualTo" @genType let gte = _gte @send external _lt: (t, t) => bool = "isLessThan" @genType let lt = _lt @send external _lte: (t, t) => bool = "isLessThanOrEqualTo" @genType let lte = _lte

But it looked a bit messy. Maybe you have another idea? I couldn't get the gentype to work on 'external' bindings.

type rec t = private { gt: t => bool, get: t => bool, }

We can have the API written in TS style, but do it in ReScript

So we can reuse the code

I like the suggestion of binging as records. Then docs/templates look the same in ts/res and js.

Also... rather make this a js file, then add a declaration file and genType.import should point to the js file. Otherwise JS users can't use this module.

But first prize is just create the api directly in rescript

js users can use bignumber.js directly?

Will do the 'record' based interface 👍

JasoonS · 2024-06-06T07:51:20Z

+            RescriptType::BigDecimal => {
+                "// default value not required since BigDecimal doesn't exist on contracts for contract import"
+                    .to_string()


Solidity smart contracts never have BigDecimals, so this isn't required.

JonoPrest · 2024-06-06T08:52:22Z

+            RescriptType::BigDecimal => {
+                "// default value not required since BigDecimal doesn't exist on contracts for contract import"
+                    .to_string()


Ok cool maybe don't do a line comment for this in case someone tries to use it later on. Then at least it can be inlined/nested and not break the editor expectations.

Suggested change

RescriptType::BigDecimal => {

"// default value not required since BigDecimal doesn't exist on contracts for contract import"

.to_string()

RescriptType::BigDecimal => {

"/*default value not required since BigDecimal doesn't exist on contracts for contract import*/"

.to_string()

I agree, but it'll break anyway since it'll expect the value :)

JonoPrest · 2024-06-06T08:55:37Z

@@ -0,0 +1,47 @@
+@genType.import(("./BigDecimal.ts", "BigDecimal")) // This feature might not work in future versions of gentype: https://rescript-lang.org/docs/manual/latest/typescript-integration#deprecated-features


It will work 🙂 I've chatted with Cristiano and CometKim and the feature will be kept for rescript 12

JonoPrest · 2024-06-06T08:59:51Z

+  SimpleNftContract,
+  NftCollectionEntity,
+  UserEntity,
+  // BigDecimal, /// For some reason this doesn't work?


@JasoonS see my comment above, this needs to be exported from index.js and index.d.ts to get these directly from generated as a package

JonoPrest

Hey @JasoonS, thanks this is awesome! Can ask if I we can merge 1r, 2r and 3r into main first and then rebase on top of that?

There will be some conflicts with the postgres types etc. I'm happy to help with those. But will be much easier than the other way around. Let me know if you want to give those a review first. Dmitry has reviewed and happy 👍🏼

JasoonS · 2024-06-07T12:11:20Z

1r, 2r and 3r into main first and then rebase on top of that?

Sure, will switch to record interface and craete an index.js file still. Shouldn't have conflicts.

…imal.res Co-authored-by: Dmitry Zakharov <dzakh.dev@gmail.com>

JasoonS · 2024-06-10T11:53:07Z

@@ -0,0 +1,38 @@
+@genType.import(("bignumber.js", "default"))


Turns out there already were typescript types for this in the library... Was doing extra work for nothing

JasoonS · 2024-06-10T11:53:47Z

+@new @module external fromBigInt: Ethers.BigInt.t => t = "bignumber.js"
+@new @module external fromFloat: float => t = "bignumber.js"
+@new @module external fromInt: int => t = "bignumber.js"
+@new @module external fromStringUnsafe: string => t = "bignumber.js"
+@new @module external fromString: string => option<t> = "bignumber.js"


These are not shared with typescript and javascript.

They should use new BigDecimal(<a value of any suitable type>)

I'm thinking maybe since it's just a binding without creating our own interface maybe we should use send bindings instead of record? What do you think @DZakh?

Sorry to mess you around Jason. I thought if we were creating our own class/types for interfacing with this it would be a good idea but since it's just bindings I would suggest we use idiomatic rescript bindings.

I agree that @send bindings are more idiomatic and flexible in ReScript since they allow for the handling of function overloaded definitions.

DZakh

The .d.ts file looks wrong. Everything else is good 👍

DZakh · 2024-06-10T12:01:16Z

+{{/each}}
+} from "./src/Enum.gen";
+
+import {default as $$t} from 'bignumber.js';


Is this needed?

Ah, no copy paste error 👍

Should be:

Suggested change

import {default as $$t} from 'bignumber.js';

import {default as BigDecimal} from 'bignumber.js';

Now I'm confused how it is working currently. I guess typscript took the types from the index.js file rather.

JasoonS · 2024-06-10T12:19:46Z

An E2E example with BigDecimal (and some silly maths on them).

JasoonS · 2024-06-10T12:30:55Z

The failing test worked locally for me. I'll check again after meeting

JasoonS · 2024-06-12T08:41:41Z

Denham - this is how to use it in typescript.

JasoonS · 2024-06-12T08:42:32Z

  };
  context.NftCollection.set(nftCollection);
+
+  context.EntityWithFields.set({ id: "testingBigDecimalWorks", bigDecimal: new BigDecimal(123.456) })


This line shows using a BigDecimal @DenhamPreen

The workflow was pinned to a v1.x-era SHA using Discord.js v11 / snekfetch, which does not truncate the `details` payload. When E2E tests failed, the action tried to send an error body over Discord's 2000-char limit, Discord returned 400 Bad Request, and snekfetch threw unhandled — so no one got the failure notification. Upstream fix: issue #15 (closed 2025-06-24), shipped in the v2.x rewrite. v2.1.1+ also downgrades webhook errors to notices, so a broken notification no longer crashes the step. Pinned to SHA c259727 (v2.2.1). Co-authored-by: Claude <noreply@anthropic.com>

JasoonS requested review from DZakh and JonoPrest June 6, 2024 07:28

JasoonS commented Jun 6, 2024

View reviewed changes

DZakh approved these changes Jun 6, 2024

View reviewed changes

JasoonS commented Jun 6, 2024

View reviewed changes

JonoPrest reviewed Jun 6, 2024

View reviewed changes

JasoonS and others added 6 commits June 10, 2024 12:56

Move float to convert to a string first

be0656b

Update indexer to include BigDecimal and add corresponding test

d4d7631

Add better typescript types and create tests

b8a54a4

Update codegenerator/cli/templates/static/codegen/src/bindings/BigDec…

1463d90

…imal.res Co-authored-by: Dmitry Zakharov <dzakh.dev@gmail.com>

Fix typegen of typescript and javascript types for BigDecimal library

1fbd8cd

Fixes after rebase and corrections

141a7e3

JasoonS force-pushed the js/big-decimal-integration branch from 5cfb184 to 141a7e3 Compare June 10, 2024 11:50

JasoonS commented Jun 10, 2024

View reviewed changes

DZakh approved these changes Jun 10, 2024

View reviewed changes

Final updates and adding idiomatic types to typescript

0aee6e0

JonoPrest force-pushed the js/big-decimal-integration branch from 56681ee to 0aee6e0 Compare June 11, 2024 07:43

Hardcode hardhat version

58a117f

JonoPrest merged commit d9b072b into main Jun 11, 2024

JasoonS commented Jun 12, 2024

View reviewed changes

		@@ -0,0 +1,111 @@
		import { expect } from 'chai';
		// import { BigDecimal } from 'generated';/// For some reason this doesn't work?

		@@ -0,0 +1,47 @@
		@genType.import(("./BigDecimal.ts", "BigDecimal")) // This feature might not work in future versions of gentype: https://rescript-lang.org/docs/manual/latest/typescript-integration#deprecated-features

		@@ -0,0 +1,38 @@
		@genType.import(("bignumber.js", "default"))

	import {default as $$t} from 'bignumber.js';
	import {default as BigDecimal} from 'bignumber.js';

Uh oh!

Conversation

JasoonS commented Jun 6, 2024

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!

DZakh left a comment

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!

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!

JasoonS Jun 7, 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!

JonoPrest left a comment

Choose a reason for hiding this comment

Uh oh!

JasoonS commented Jun 7, 2024

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!

DZakh 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!

Choose a reason for hiding this comment

Uh oh!

JasoonS commented Jun 10, 2024

Uh oh!

JasoonS commented Jun 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasoonS Jun 7, 2024 •

edited

Loading