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

[WIP][TS SDK] integrate ANS functions #6000

Closed
wants to merge 6 commits into from
Closed

[WIP][TS SDK] integrate ANS functions #6000

wants to merge 6 commits into from

Conversation

0xmaayan
Copy link
Contributor

Description

  1. Integrates ANS functions into the TS SDK and adds unit tests.
  1. Moving plugin files into a plugin folder and keep all tests under __tests__ folder.

Test Plan

tests are passing

Copy link
Contributor

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

Looks like it's coming along well.

I don't think you need to include all the bytecode modules do you? Probably can be built and used at test time

@banool
Copy link
Contributor

banool commented Jan 4, 2023

If we're adding compiled bytecode we should also add CI that ensures the bytecode doesn't fall out of sync when the source code changes. If you can collect the commands you used to generate the bytecode in the first place I can help with that.

@0xmaayan
Copy link
Contributor Author

0xmaayan commented Jan 4, 2023

The way I did it is

  1. compiled the files locally
  2. add those files to the folder
  3. read the compiled files and use in the test
    https://github.com/aptos-labs/aptos-core/pull/6000/files#diff-af27ba8aea8ccbfc871dd0e005811cc7198ad93d52686f017e79bc15483c96bdR46-R58

@gregnazario if I dont include the files, where should I read it from?
@banool this is the command I used https://github.com/aptos-labs/aptos-core/pull/6000/files#diff-f1581aea77c19cf634bd04e4231eeba3c093d4d253c8999a135f72670c709c74R17 it is in a readme file that explains how to use it in tests

@0xmaayan
Copy link
Contributor Author

0xmaayan commented Jan 5, 2023

If we're adding compiled bytecode we should also add CI that ensures the bytecode doesn't fall out of sync when the source code changes. If you can collect the commands you used to generate the bytecode in the first place I can help with that.

I am not sure it would have any affect if the source code is changed. We compile the source code locally, and publish those files during tests, so the test knows about the local compiled files and not the changed source code

@banool
Copy link
Contributor

banool commented Jan 5, 2023

I am not sure it would have any affect if the source code is changed. We compile the source code locally, and publish those files during tests, so the test knows about the local compiled files and not the changed source code

I'm saying that in CI, we should compile the source code and make sure the bytecode matches the bytecode checked in. This is how we do it in aptos framework for example.

@banool
Copy link
Contributor

banool commented Jan 13, 2023

Okay we spoke offline with Kevin and Jijun, probably better to go with a remote ABI downloading approach rather than a hardcode + CI approach.

@0xmaayan
Copy link
Contributor Author

Okay we spoke offline with Kevin and Jijun, probably better to go with a remote ABI downloading approach rather than a hardcode + CI approach.

Yes, still need a way to compile and publish this package (or inject it when building the local testnet), this package is not part of the aptos core framework

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

should this be both read and write?

@gregnazario
Copy link
Contributor

If we can, let's resurrect this as just the read for lookups, it should be relatively simple (don't need all the entry functions).

Then, we can come back on another pass to make writes & creating of names

@davidiw
Copy link
Contributor

davidiw commented Mar 19, 2023

#7227
looks like this is stale

@davidiw davidiw closed this Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants