-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(tokenCreateTransaction): Implement TokenCreateTransaction
E2E tests: TCK
#252
base: main
Are you sure you want to change the base?
feat(tokenCreateTransaction): Implement TokenCreateTransaction
E2E tests: TCK
#252
Conversation
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
Signed-off-by: Rob Walworth <[email protected]>
a42c641
to
0bc413e
Compare
ef72ffa
to
a42c641
Compare
a42c641
to
f5f27cc
Compare
…erging with main Signed-off-by: ivaylogarnev-limechain <[email protected]>
…reatetransaction Signed-off-by: ivaylogarnev-limechain <[email protected]>
Signed-off-by: ivaylogarnev-limechain <[email protected]>
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.
As we continue to expand the test suite, we currently have around 150 tests for account transactions. With the PR for the token create transaction, the number of tests has increased to about 400. With the addition of other update/delete operations in the token transaction series, I anticipate the total will exceed 700 tests.
Some code is repeatable, as expected. A quick example is the Keys series of tests for TokenCreate transaction, which has about 2400 lines, where the tests are largely the same except for different key names being tested. As this scales over time, it becomes crucial to think about introducing helper functions or even templates to save time in future development.
Additionally, some other tests across different transactions also show repeatable patterns, such as tests for the Name property or the Auto Renew Period. Introducing templates or other optimization techniques could streamline these tests as well.
}); | ||
}); | ||
|
||
describe("Custom Fees", function () { |
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.
We can:
- Extract "Custom Fee" helper functions to different file in the helpers folder.
- Merge common fee comparison logic with creating a generic helper functions to handle common comparison logic.
- Write a single verifyTokenCreation function that handles the logic for comparing custom fees for different types, making it modular.
- Call this general function for different fee types (Fixed, Fractional, Royalty) by providing the appropriate compare functions and types
The goal here is to enhance reusability and modularity, ensuring the code can be easily maintained and extended for future use.
/** | ||
* Tests for TokenCreateTransaction | ||
*/ | ||
describe.only("TokenCreateTransaction", function () { |
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.
Delete .only, it was mistakenly forgotten.
@@ -0,0 +1,6 @@ | |||
import { TokenSupplyType } from "@hashgraph/sdk"; |
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.
- This should be TokenType, also please check why the "Token Type" test suite is passing.
- Also we are passing tokenType as a "nft" or "ft", shouldn't we use the TokenType class?
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 was trying to restrict use of the JS SDK as much as I could, I feel like since this is meant to test the JS SDK it should keep its dependence on it to a minimum. We could use another enum to track "nft"
and "ft"
token types, but the JSON sent across is a string so it must be one of these two options
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 agree that avoiding the use of the JS SDK and the TokenType class is the better approach. However, based on my personal experience, it might be a bit confusing for a new team member who has previously used TokenType to create a TokenCreateTransaction. Since it can be stringified using .toString(), it could still be used in this context. That said, I understand that simplifying things and steering clear of direct JS SDK usage will allow us to focus more on the tests rather than on the SDK itself.
In short, let's stick with the current approach.
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.
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?
assert.fail("Should throw an error"); | ||
}); | ||
|
||
it("(#3) Creates a token with an expiration time of 9,223,372,036,854,775,807 (int64 max) seconds", async function () { |
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 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?
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.
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).
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.
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.
What is the reason this class enforces these constraints? Those seem like very arbitrary bounds
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 believe this test should pass now that we've converted the BigInts to strings and pass Long.fromString() here.
@@ -36,10 +36,19 @@ class ConsensusInfoClient { | |||
return this.executeAccountMethod(accountId, new AccountInfoQuery()); | |||
} | |||
|
|||
async getTokenInfo(tokenId) { | |||
return this.executeTokenMethod(tokenId, new TokenInfoQuery()); |
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.
Import TokenInfoQuery here
assert.fail("Should throw an error"); | ||
}); | ||
|
||
it("(#5) Creates a token with an auto renew period of 9,223,372,036,854,775,808 (`int64` max + 1) seconds", async function () { |
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.
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
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.
Yes, it's the same in JavaScript. The test isn't throwing an error because the period is being ignored.
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.
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).
name: "testname", | ||
symbol: "testsymbol", | ||
treasuryAccountId: process.env.OPERATOR_ACCOUNT_ID, | ||
expirationTime: Date.now() / 1000 + 8000002, |
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.
this number is of type double
but it should not have floating point.
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.
and after rounding the number the transaction goest through, but the assert is otherwise
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.
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
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.
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.
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.
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.
assert.fail("Should throw an error"); | ||
}); | ||
|
||
it("(#5) Creates a token with an expiration time of 9,223,372,036,854,775,808 (int64 max + 1) seconds", async function () { |
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.
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.
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.
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 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?
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.
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.
…O, removed the TokenType enum and other minor things Signed-off-by: ivaylogarnev-limechain <[email protected]>
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 |
…efix for KeyList Signed-off-by: ivaylogarnev-limechain <[email protected]>
…d the returned from the test key Signed-off-by: ivaylogarnev-limechain <[email protected]>
Signed-off-by: ivaylogarnev-limechain <[email protected]>
…plement-json-rpc-method-for-tokencreatetransaction Signed-off-by: ivaylogarnev-limechain <[email protected]>
Signed-off-by: ivaylogarnev-limechain <[email protected]>
d304ea6
to
f1ed3fa
Compare
Description:
This PR implements the
TokenCreateTransaction
tests documented intest-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 becausedeleteToken
is not yet implemented.Related issue(s):
Fixes #231 #244
Checklist