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.

Sign up for GitHub

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

feat: transaction with CONTRACT_NEGATIVE_VALUE breaks some routes #3387

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: transaction with CONTRACT_NEGATIVE_VALUE breaks some routes #3387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bde9a91
chore: handle contract negative value calls
natanasow Jan 14, 2025
ec6dc59
chore: remove .only
natanasow Jan 14, 2025
48ce0a3
chore: fix comments
natanasow Jan 14, 2025
c17f02a
chore: edit imports
natanasow Jan 15, 2025
5e9e1b1
chore: resolving comments
natanasow Jan 16, 2025
fa9f894
chore: fix json parsing
natanasow Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/relay/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@
},
"dependencies": {
"@ethersproject/asm": "^5.7.0",
"@hashgraph/sdk": "^2.54.0-beta.1",
"@hashgraph/json-rpc-config-service": "file:../config-service",
"@hashgraph/sdk": "^2.54.0-beta.1",
"@keyvhq/core": "^1.6.9",
"axios": "^1.4.0",
"axios-retry": "^3.5.1",
Expand All @@ -60,6 +60,7 @@
"dotenv": "^16.0.0",
"ethers": "^6.7.0",
"find-config": "^1.0.0",
"json-bigint": "^1.0.0",
"keccak": "^3.0.2",
"keyv": "^4.2.2",
"keyv-file": "^0.3.0",
Expand Down
52 changes: 46 additions & 6 deletions packages/relay/src/formatters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@
*
*/

import { ConfigService } from '@hashgraph/json-rpc-config-service/dist/services';
import { BigNumber } from '@hashgraph/sdk/lib/Transfer';
import { BigNumber as BN } from 'bignumber.js';
import crypto from 'crypto';

import constants from './lib/constants';
import { BigNumber as BN } from 'bignumber.js';
import { BigNumber } from '@hashgraph/sdk/lib/Transfer';
import { ConfigService } from '@hashgraph/json-rpc-config-service/dist/services';
import { Transaction, Transaction1559, Transaction2930 } from './lib/model';

const EMPTY_HEX = '0x';
Expand Down Expand Up @@ -178,7 +179,7 @@
transactionIndex: nullableNumberTo0x(cr.transaction_index),
type: cr.type === null ? '0x0' : nanOrNumberTo0x(cr.type),
v: cr.v === null ? '0x0' : nanOrNumberTo0x(cr.v),
value: nanOrNumberTo0x(tinybarsToWeibars(cr.amount)),
value: nanOrNumberInt64To0x(tinybarsToWeibars(cr.amount, true)),
// for legacy EIP155 with tx.chainId=0x0, mirror-node will return a '0x' (EMPTY_HEX) value for contract result's chain_id
// which is incompatibile with certain tools (i.e. foundry). By setting this field, chainId, to undefined, the end jsonrpc
// object will leave out this field, which is the proper behavior for other tools to be compatible with.
Expand Down Expand Up @@ -265,6 +266,34 @@
return input == null || Number.isNaN(input) ? numberTo0x(0) : numberTo0x(input);
};

const nanOrNumberInt64To0x = (input: number | BigNumber | bigint | null): string => {
// converting to string and then back to int is fixing a typescript warning
if (input && Number(input) < 0) {
// the hex of a negative number can be obtained from the binary value of that number positive value
// the binary value needs to be negated and then to be incremented by 1

// how the transformation works (using 16 bits)
// a 16 bits integer variables have values from -32768 to +32767, so:
// 0 - 0x0000 - 0000 0000 0000 0000
// 32767 - 0x7fff - 0111 1111 1111 1111
// -32768 - 0x8000 - 1000 0000 0000 0000
// -1 - 0xffff - 1111 1111 1111 1111

// converting int16 -10 will be done as following:
// - make it positive = 10
// - 16 bits binary value of 10 = 0000 0000 0000 1010
// - inverse the bits = 1111 1111 1111 0101
// - adding +1 = 1111 1111 1111 0110
// - 1111 1111 1111 0110 bits = 0xfff6

// we're using 64 bits integer because that's the type returned by the mirror node - int64
const bits = 64;
return numberTo0x((BigInt(input.toString()) + (BigInt(1) << BigInt(bits))) % (BigInt(1) << BigInt(bits)));

Check warning on line 291 in packages/relay/src/formatters.ts

View check run for this annotation

Codecov / codecov/patch

packages/relay/src/formatters.ts#L290-L291 

Added lines #L290 - L291 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

question: do we expect number bigger than 53 bits? so it's not automatically coerced to double https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I don't know how often it will happen but we definitely must handle values bigger than Number.MAX_SAFE_INTEGER.

The max Int64 positive value is 0x7fffffffffffffff which has 63 "usable" bits.

0111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111

That's why we always convert the incoming value to BigInt here 👇

BigInt(input.toString())

Hope that makes sense or am I wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, I'm not sure that BigInt(input.toString()) works as expected when input being a number uses more than 53 bits. Say

> const x = 0x7fffffffffffffff
> typeof(x)
'number'
> BigInt(x.toString()) - 0x7fffffffffffffffn
193n

but the difference should be zero, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, the difference should be zero. Nice catch 👏 and thank you. That's something I must add more unit tests tomorrow.

Here are some more quick tests, it seems that .toString() messed things up, also using a shadow conversion like postfixing n doesn't seem to work as expected.

> const x = 0x7fffffffffffffff
undefined
> typeof(x)
'number'
> BigInt(x.toString()) - 0x7fffffffffffffffn
193n
> BigInt(x) - 0x7fffffffffffffffn
1n
> BigInt(x) - BigInt(0x7fffffffffffffff)
0n
> BigInt(x) - BigInt(9223372036854776000)
0n

Do you have an idea how this can be completely fixed? I think removing .toString() is enough but that will be confirmed when I push the unit tests covering wide-bit range numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think removing .toString() is enough

The problem is with number altogether.

> BigInt(0x7fffffffffffffff) - 0x7fffffffffffffffn
1n

The only way I see to proper fix it completely would be to not use number, and use bigint everywhere for values that are int64.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. Will try to refactor it in an elegant way and once I'm done will ping you again for review.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @acuarica,

Richard Moore has the same thoughts as you ethers-io/ethers.js#452 (comment). Today I managed to resolve it but what do you think about the implementation?

What I've done:

  • decided to use json-bigint
  • hooked up into axios's transformResponse method
  • parsed the json string before it's been parsed by the default JSON.parse where the number will be rounded and lost forever

Here are the two mainnet transactions I tested with:

Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the advantage of using json-bigint over bigint? Only parsing? doesn't bigint already comes with parsing capabilities?

Copy link
Collaborator Author

@natanasow natanasow Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

json-bigint is doing JSONBigInt.parse(data) but under the hood, it transforms numeric values to bigint instead of JS Number.

If we're doing JSON.parse() and then manually transforming numeric values to bigint, JSON.parse() already rounded the number and we lost it.

json-bigint doesn't replace bigint and they are not correlated. json-bigint is used instead of JSON.parse() due to its capability of converting string json input directly to bigint instead of JS Number.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification.

}

return nanOrNumberTo0x(input);
};

const toHash32 = (value: string): string => {
return value.substring(0, 66);
};
Expand Down Expand Up @@ -303,8 +332,18 @@
return data.replace(/^0x/, '').substring(0, 8);
};

const tinybarsToWeibars = (value: number | null) => {
if (value && value < 0) throw new Error('Invalid value - cannot pass negative number');
const tinybarsToWeibars = (value: number | null, allowNegativeValues: boolean = false) => {
if (value && value < 0) {
// negative amount can be received only by CONTRACT_NEGATIVE_VALUE revert
// e.g. tx https://hashscan.io/mainnet/transaction/1735241436.856862230
// that's not a valid revert in the Ethereum world so we must NOT multiply
// the amount sent via CONTRACT_CALL SDK call by TINYBAR_TO_WEIBAR_COEF
// also, keep in mind that the mirror node returned amount is typed with int64
if (allowNegativeValues) return value;

throw new Error('Invalid value - cannot pass negative number');

Check warning on line 344 in packages/relay/src/formatters.ts

View check run for this annotation

Codecov / codecov/patch

packages/relay/src/formatters.ts#L344 

Added line #L344 was not covered by tests
}

if (value && value > constants.TOTAL_SUPPLY_TINYBARS)
throw new Error('Value cannot be more than the total supply of tinybars in the blockchain');

Expand All @@ -324,6 +363,7 @@
numberTo0x,
nullableNumberTo0x,
nanOrNumberTo0x,
nanOrNumberInt64To0x,
toHash32,
toNullableBigNumber,
toNullIfEmptyHex,
Expand Down
23 changes: 23 additions & 0 deletions packages/relay/src/lib/clients/mirrorNodeClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import { ethers } from 'ethers';
import http from 'http';
import https from 'https';
import JSONBigInt from 'json-bigint';
import { Logger } from 'pino';
import { Histogram, Registry } from 'prom-client';

Expand Down Expand Up @@ -361,6 +362,28 @@
if (pathLabel == MirrorNodeClient.GET_CONTRACTS_RESULTS_OPCODES) {
response = await this.web3Client.get<T>(path, axiosRequestConfig);
} else {
// JS supports up to 53 bits integers, once a number with more bits is converted to a js Number type, the initial value is already lost
// in order to fix that, we have to use a custom JSON parser that hooks up before
// the default axios JSON.parse conversion where the value will be rounded and lost
// JSONBigInt reads the string representation of received JSON and formats each number to BigNumber
axiosRequestConfig['transformResponse'] = [
(data) => {
// if the data is not valid, just return it to stick to the current behaviour
if (data) {
try {
// try to parse it, if the json is valid, numbers within it will be converted
// this case will happen on almost every GET mirror node call
return JSONBigInt.parse(data);
} catch (e) {
// in some unit tests, the mocked returned json is not property formatted
// so we have to preprocess it here with JSON.stringify()
return JSONBigInt.parse(JSON.stringify(data));

Check warning on line 380 in packages/relay/src/lib/clients/mirrorNodeClient.ts

View check run for this annotation

Codecov / codecov/patch

packages/relay/src/lib/clients/mirrorNodeClient.ts#L380 

Added line #L380 was not covered by tests
}
}

return data;

Check warning on line 384 in packages/relay/src/lib/clients/mirrorNodeClient.ts

View check run for this annotation

Codecov / codecov/patch

packages/relay/src/lib/clients/mirrorNodeClient.ts#L384 

Added line #L384 was not covered by tests
},
];
response = await this.restClient.get<T>(path, axiosRequestConfig);
}
} else {
Expand Down
110 changes: 92 additions & 18 deletions packages/relay/tests/lib/formatters.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@
*
*/

import { BigNumber as BN } from 'bignumber.js';
import { expect } from 'chai';
import { AbiCoder, keccak256 } from 'ethers';

import {
ASCIIToHex,
decodeErrorMessage,
Expand All @@ -31,23 +34,22 @@ import {
isHex,
isValidEthereumAddress,
mapKeysAndValues,
nanOrNumberInt64To0x,
nanOrNumberTo0x,
nullableNumberTo0x,
numberTo0x,
parseNumericEnvVar,
prepend0x,
strip0x,
tinybarsToWeibars,
toHash32,
toHexString,
toNullableBigNumber,
toNullIfEmptyHex,
trimPrecedingZeros,
weibarHexToTinyBarInt,
tinybarsToWeibars,
} from '../../src/formatters';
import constants from '../../src/lib/constants';
import { BigNumber as BN } from 'bignumber.js';
import { AbiCoder, keccak256 } from 'ethers';
import { overrideEnvsInMochaDescribe } from '../helpers';

describe('Formatters', () => {
Expand Down Expand Up @@ -399,6 +401,72 @@ describe('Formatters', () => {
});
});

describe('nanOrNumberInt64To0x', () => {
it('should return 0x0 for nullable input', () => {
expect(nanOrNumberInt64To0x(null)).to.equal('0x0');
});
it('should return 0x0 for NaN input', () => {
expect(nanOrNumberInt64To0x(NaN)).to.equal('0x0');
});
it('should convert negative int64 number (2 digits)', () => {
expect(nanOrNumberInt64To0x(BigInt('-10'))).to.equal('0xfffffffffffffff6');
});
it('should convert negative int64 number (6 digits)', () => {
expect(nanOrNumberInt64To0x(BigInt('-851969'))).to.equal('0xfffffffffff2ffff');
});
it('should convert negative int64 number (19 digits -6917529027641081857)', () => {
expect(nanOrNumberInt64To0x(BigInt('-6917529027641081857'))).to.equal('0x9fffffffffffffff');
});
it('should convert negative int64 number (19 digits -9223372036586340353)', () => {
expect(nanOrNumberInt64To0x(BigInt('-9223372036586340353'))).to.equal('0x800000000fffffff');
});
it('should convert positive 10 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('593'))).to.equal('0x251');
});
it('should convert positive 50 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('844424930131967'))).to.equal('0x2ffffffffffff');
});
it('should convert positive 51 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('1970324836974591'))).to.equal('0x6ffffffffffff');
});
it('should convert positive 52 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('3096224743817215'))).to.equal('0xaffffffffffff');
});
it('should convert positive 53 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('9007199254740991'))).to.equal('0x1fffffffffffff');
});
it('should convert positive 54 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('13510798882111487'))).to.equal('0x2fffffffffffff');
});
it('should convert positive 55 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('31525197391593471'))).to.equal('0x6fffffffffffff');
});
it('should convert positive 56 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('49539595901075455'))).to.equal('0xafffffffffffff');
});
it('should convert positive 57 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('144115188075855871'))).to.equal('0x1ffffffffffffff');
});
it('should convert positive 58 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('216172782113783807'))).to.equal('0x2ffffffffffffff');
});
it('should convert positive 59 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('504403158265495551'))).to.equal('0x6ffffffffffffff');
});
it('should convert positive 60 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('792633534417207295'))).to.equal('0xaffffffffffffff');
});
it('should convert positive 61 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('2305843009213693951'))).to.equal('0x1fffffffffffffff');
});
it('should convert positive 62 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('3458764513820540927'))).to.equal('0x2fffffffffffffff');
});
it('should convert positive 63 bits number', () => {
expect(nanOrNumberInt64To0x(BigInt('8070450532247928831'))).to.equal('0x6fffffffffffffff');
});
});

describe('toHash32', () => {
it('should format more than 32 bytes hash to 32 bytes', () => {
expect(
Expand Down Expand Up @@ -735,27 +803,33 @@ describe('Formatters', () => {
});

describe('tinybarsToWeibars', () => {
it('should convert tinybars to weibars', () => {
expect(tinybarsToWeibars(10)).to.eql(100000000000);
});
for (const allowNegativeValues of [true, false]) {
it(`should convert tinybars to weibars allowNegativeValues = ${allowNegativeValues}`, () => {
expect(tinybarsToWeibars(10, allowNegativeValues)).to.eql(100000000000);
});

it('should return null if null is passed', () => {
expect(tinybarsToWeibars(null)).to.eql(null);
});
it(`should return null if null is passed allowNegativeValues = ${allowNegativeValues}`, () => {
expect(tinybarsToWeibars(null, allowNegativeValues)).to.eql(null);
});

it('should return 0 for 0 input', () => {
expect(tinybarsToWeibars(0)).to.eql(0);
});
it(`should return 0 for 0 input allowNegativeValues = ${allowNegativeValues}`, () => {
expect(tinybarsToWeibars(0, allowNegativeValues)).to.eql(0);
});

it(`should throw an error when value is larger than the total supply of tinybars allowNegativeValues = ${allowNegativeValues}`, () => {
expect(() => tinybarsToWeibars(constants.TOTAL_SUPPLY_TINYBARS * 10, allowNegativeValues)).to.throw(
Error,
'Value cannot be more than the total supply of tinybars in the blockchain',
);
});
}

it('should throw an error when value is smaller than 0', () => {
expect(() => tinybarsToWeibars(-10)).to.throw(Error, 'Invalid value - cannot pass negative number');
expect(() => tinybarsToWeibars(-10, false)).to.throw(Error, 'Invalid value - cannot pass negative number');
});

it('should throw an error when value is larger than the total supply of tinybars', () => {
expect(() => tinybarsToWeibars(constants.TOTAL_SUPPLY_TINYBARS * 10)).to.throw(
Error,
'Value cannot be more than the total supply of tinybars in the blockchain',
);
it('should return the negative number if allowNegativeValues flag is set to true', () => {
expect(tinybarsToWeibars(-10, true)).to.eql(-10);
});
});
});
Loading