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.

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: implement getTokenInfo for Hardhat #142

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

Conversation

acuarica
Copy link
Contributor

Description:

This PR implements getTokenInfo for Hardhat. It extends the getHtsStorageAt to interpret Solidity structs and arrays together with support for more types.

Related issue(s):

Fixes #141.

Notes for reviewer:

Checklist

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

@acuarica acuarica added the feature Enhancing an existing feature driven by business requirements. Typically backwards compatible. label Nov 27, 2024
@acuarica acuarica self-assigned this Nov 27, 2024
@acuarica acuarica requested a review from a team as a code owner November 27, 2024 08:24
@acuarica acuarica linked an issue Nov 27, 2024 that may be closed by this pull request
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
@hts-forking/src/slotmap.js Outdated Show resolved Hide resolved
Comment on lines +106 to +109
const customFees = /**@type {Record<string, unknown>}*/ (token['custom_fees']);
token['fixed_fees'] = customFees['fixed_fees'] ?? [];
token['fractional_fees'] = customFees['fractional_fees'] ?? [];
token['royalty_fees'] = customFees['royalty_fees'] ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this would work, because the actual TokenInfo struct has very different fields for the custom fees from those which are in the mirror node response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, true that. Going to fix it.

Copy link
Contributor Author

@acuarica acuarica Nov 27, 2024

Choose a reason for hiding this comment

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

I think it would be better to tackle this in another PR. The reason being, our tests don't cover custom fees. This implies creating custom tokens with the desired fees, which in itself seems out of scope. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, sounds good.

Btw, what I did to test things out for the custom fees (while doing the foundry solution) was to mock some data inside the json files for USDC and MFCT tokens and compare the results from getTokenInfo with the data

token['default_kyc_status'] = false;
token['treasury'] = token['treasury_account_id'];
token['ledger_id'] = '0x00';
token['second'] = `${token['expiry_timestamp']}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this supposed to be part of the expiry struct? Why are we mapping it directly to a second field? Also, what about the auto-renewal account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, but these structs are flattened, so it uses the last part of field path, i.e., .second.

For the same reasons, the auto-renewal fields didn't need any extra care.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, makes sense. Let's add a small comment to clarify this.

* @param {Record<string, unknown>} token
* @returns {Map<bigint, {value: string, path: string}>}
*/
function slotMapOf(token) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method seems really hacky and tries to hit multiple rabbits with a single bullet. The flatten method also seems to have multiple purposes and is really hard to read and follow through.

Wouldn't it be better if we just constructed a new object that would represent the TokenInfo struct and mapped the mirror node response to it?

For example, one way to do it can be by defining a new class that will represent the TokenInfo struct:

  • in its constructor (or through a static method - e.g., fromMirrorNodeToken) it can be created from the mirror node object - something similar to what is done in _initTokenInfo in the foundry solution where the TokenInfo struct is being populated from the mirror node JSON response.

Then in order to fill the slots we would have to:

  1. Construct an instance of the TokenInfo object, by populating it from the token response from mirror node
  2. Iterate through the fields of the TokenInfo object and use the storage layout to figure out the slots where each field should be residing

This seems way easier to understand and also would be very similar to what is done in the HtsSystemContractJson._initTokenInfo method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm, so 1. and 2. is essentially what slotMapOf is doing. The first bit corresponds loosely to 1., but just amending the token so it's compatible with the Solidity definition. The second part is doing 2., but driven by the storage layout instead.

getHtsStorageAt is driven by slot number (not fields), so to me it's more clear to follow the storage layout first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, can we create a new object, instead of amending the token object? Also, in the new object we can directly write all fields explicitly in camelCase (as they are in the solidity struct), instead of snake case (this would skip the conversion logic).

In this way it would be more explicit and clearer what fields exist in the mapped object and what we are doing, amending the token object is kind of ambiguous because we are only adding the missing fields and requires us to believe that the actual response will contain the rest of the fields, instead of catching any potential problems (missing fields, etc) on time.

Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Enhancing an existing feature driven by business requirements. Typically backwards compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Hardhat] Implement getTokenInfo state on HTS for Hardhat
2 participants