feat(tokenCreateTransaction): Implement TokenCreateTransaction E2E tests: TCK

[rwalworth]

Description:
This PR implements the TokenCreateTransaction tests documented in test-specifications/token-service/tokenCreateTransaction.md. Currently three tests are skipped because of a bug in the services code which allows for tokens to be created with a custom fee that collects to a deleted account. Another test is skipped because deleteToken is not yet implemented.

Related issue(s):

Fixes #231 #244

Checklist

• Documented (Code comments, README, etc.)
• Tested (unit, integration, etc.)

[0xivanov]

Almost all the asserts for ecdsa keys are not passing because the consensus client is DER encoding the key differently than the Java SDK, I guess we could rework the logic to assert the keys raw?
@rwalworth @ivaylogarnev-limechain WDYT?

[0xivanov]

I am unable to create an Instant from such big number
java.time.DateTimeException: Instant exceeds minimum or maximum instant.
Can we keep the same test scenarios as with account update method?

[jsync-swirlds]

Why would you need to create an Instant? The field in the API is an integer, not an Instant.
This test is intended to verify that the SDK is correctly handling the edges of the integer value, and should be retained (in my opinion).

[0xivanov]

The Java SDK currently has it's expirationTime as an java.time.Instant.
This class has these constraints:

In theory it should support seconds of int64 max value but it actually does not...

[rwalworth]

What is the reason this class enforces these constraints? Those seem like very arbitrary bounds

[ivaylogarnev-limechain]

I believe this test should pass now that we've converted the BigInts to strings and pass Long.fromString() here.

[0xivanov]

There is the following check in services:

where NA = Long.MIN_VALUE; and the autorenew period gets ignored in this test scenario.
@rwalworth @jsync-swirlds @ivaylogarnev-limechain

[ivaylogarnev-limechain]

Yes, it's the same in JavaScript. The test isn't throwing an error because the period is being ignored.

[jsync-swirlds]

This seems like a bug in services code. It should not be possible to set a value in the SDK that results in services ignoring the value as "unset".
If the services code needs to know if a value is or is not set in protobuf, we need to modify PBJ to support proto3 "explicit presence". Currently an unset value just gets the field-type default (0 in this case).

[0xivanov]

this number is of type double but it should not have floating point.

[0xivanov]

and after rounding the number the transaction goest through, but the assert is otherwise

[ivaylogarnev-limechain]

We will modify how we handle the time values by converting them to ISO string format, as the current method for processing times is incorrect.

example for this specific test:

        const expirationDate = new Date(Date.now() + 8000002000);
        const expirationTimeIso = expirationDate.toISOString();

And we will send the expirationTimeIso

[rwalworth]

This one is incorrect yes, it should be wrapped in parseInt, but how have others been incorrect? Just for my own edification. I'm not sure if we want to send these across as strings as it would just involve extra processing on the SDK side to convert from a string to a number/integer, then to a time/Instant/etc.

[ivaylogarnev-limechain]

The problem occurs when we attempt to add or remove time using numbers alone. When we create a new Date() on the server (SDK) with these numeric values, it often results in an invalid date. This invalid date is then passed to the setter, causing an 'INVALID_EXPIRATION_TIME' error. This error leads to the test passing, which is not the expected behavior.

On the other hand, using ISO string format for the expiration time is a more reliable approach. Since ISO strings represent a standardized date format, they can't produce incorrect or invalid date values, ensuring that the expiration time is processed correctly while sending it as a string.

[rwalworth]

Understood, if the JS SDK is unable to parse a BigInt or Long into a Date then I'm okay with this move. Since we're changing the type of expirationTime, be sure to update the test specifications with the new type, as well as a description specifying it should be the ISO string format. I'll make an issue to change expirationTime parameters in other request types as well.

[rwalworth]

#259

[0xivanov]

same as https://github.com/hashgraph/hedera-sdk-tck/pull/252/files#r1794710905

[0xivanov]

Aren't the test cases with int64 MAX +1 and int64 MIN the same?
I mean I am handling the overflow of the numbers in my TCK server, because i expect an Integer and not BigInteger

        Optional<Long> parsedExpirationTime;
        try {
            parsedExpirationTime = Optional.ofNullable((Long) jrpcParams.get("expirationTime")); // handle long boundary
        } catch (Exception e) {
            parsedExpirationTime = Optional.of(((BigInteger) jrpcParams.get("expirationTime")).longValue()); // will overflow to negative number
        }

I am still a big confused if we really need int64 MAX +1 cases.

@rwalworth @ivaylogarnev-limechain @jsync-swirlds

[rwalworth]

I believe the idea here was just to make sure the SDKs and services could handle parsing to/from protobuf, which supports unsigned integers while services/Java doesn't. Also not sure how important it would be to make sure the SDK could parse different numbers with the same underlying bit structure. I don't feel strongly either way about keeping or removing, @jsync-swirlds you have an opinion?

[jsync-swirlds]

We need these "out of bounds" edge case tests so that the TCK ensures that all SDKs handle these values (which are valid in some languages) correctly and consistently.

Protocol Buffers do not translate these to BigInteger for Java, they translate as twos-compliment integers (so INT64_MAX + 1 is, indeed, bitwise identical to INT64_MIN), but they do not translate the same in other SDK languages.
It seems, to me, that anywhere that different SDKs are expected to correctly interpret the same protocol buffer serialized bytes in different ways are worth testing.

[rwalworth]

Almost all the asserts for ecdsa keys are not passing because the consensus client is DER encoding the key differently than the Java SDK, I guess we could rework the logic to assert the keys raw? @rwalworth @ivaylogarnev-limechain WDYT?

I am okay with switching to just comparing raw keys, though we should do this for all key comparisons in all tests. I created an issue to track this #257

[ivaylogarnev-limechain]

Please avoid checking this file, as the necessary updates should be handled here.