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: token-group interface js library #6014

Merged
merged 9 commits into from
Jan 5, 2024

Conversation

qiweiii
Copy link
Contributor

@qiweiii qiweiii commented Dec 20, 2023

closes: #5648

based on impl in #5228, added js library for token-group interface

@mergify mergify bot added the community Community contribution label Dec 20, 2023
@buffalojoec buffalojoec self-requested a review December 20, 2023 18:13
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Hey this looks great! Just take a look at my recent commit here and update your tests if you wouldn't mind!

.github/workflows/pull-request-js.yml Outdated Show resolved Hide resolved
token-group/js/src/state/tokenGroup.ts Outdated Show resolved Hide resolved
@qiweiii qiweiii requested a review from buffalojoec December 21, 2023 12:17
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Let's just tighten up the packing/unpacking and this should be good to go!

token-group/js/src/state/tokenGroup.ts Outdated Show resolved Hide resolved
token-group/js/src/state/tokenGroupMember.ts Outdated Show resolved Hide resolved
@qiweiii qiweiii requested a review from buffalojoec December 22, 2023 01:25
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

This looks great! I noticed a few tiny things, and then we can wrap it.

token-group/js/src/instruction.ts Outdated Show resolved Hide resolved
token-group/js/src/state/tokenGroup.ts Outdated Show resolved Hide resolved
token-group/js/test/state.test.ts Outdated Show resolved Hide resolved
@buffalojoec
Copy link
Contributor

Also might have to refresh the lockfile with a pnpm i.

@qiweiii
Copy link
Contributor Author

qiweiii commented Jan 4, 2024

When I run pnpm test locally in project root, it gives an error:

> @solana/[email protected] test /Users/qiweiyang/GitHub/solana-program-library/single-pool/js/packages/classic
> sed -i '1s/.*/{ "type": "module",/' package.json && NODE_OPTIONS='--loader=tsx' ava ; ret=$?; sed -i '1s/.*/{/' package.json && exit $ret

sed: 1: "package.json": extra characters at the end of p command
sed: 1: "package.json": extra characters at the end of p command

not sure if this will pass in ci

@buffalojoec
Copy link
Contributor

When I run pnpm test locally in project root, it gives an error:

> @solana/[email protected] test /Users/qiweiyang/GitHub/solana-program-library/single-pool/js/packages/classic
> sed -i '1s/.*/{ "type": "module",/' package.json && NODE_OPTIONS='--loader=tsx' ava ; ret=$?; sed -i '1s/.*/{/' package.json && exit $ret

sed: 1: "package.json": extra characters at the end of p command
sed: 1: "package.json": extra characters at the end of p command

not sure if this will pass in ci

I think what's happening is when you're merging master into your branch to resolve the merge conflicts, your lockfile is getting bungled.

I can clone your branch and run pnpm i in the root and it seems to resolve the lockfile problems. Then I immediately can run pnpm test in the Token Group JS package and it works no problem!

Maybe just try pnpm i in the root and commit the lockfile?

Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Thanks for shippin' this in!

shiny

@buffalojoec buffalojoec merged commit 480c6d9 into solana-labs:master Jan 5, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create SPL Token Group JS Library
2 participants