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

Added programId field in TokenBalance type #3592

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

shivaji43
Copy link
Contributor

@shivaji43 shivaji43 commented Nov 18, 2024

Tackles the issue (#3091)
Screenshot 2024-11-19 234833
Screenshot 2024-11-19 233507
Screenshot 2024-11-19 233442

Done proper linting , built and complied properly , also tested to see if programId returns as expected ,
also added the screenshots

Please tell me if I am missing something and Thank you for being patient and helping me through this.

Copy link

changeset-bot bot commented Nov 18, 2024

⚠️ No Changeset found

Latest commit: ca9049f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mergify mergify bot added the community label Nov 18, 2024
@mergify mergify bot requested a review from a team November 18, 2024 15:57
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Adding this property to the TypeScript type is insufficient to vend it to the caller.

Look here:

https://github.com/solana-labs/solana-web3.js/blob/maintenance/v1.x/src/connection.ts#L2329-L2334

To get your PRs in faster, make sure to test your changes, to write down in your PR description exactly what you did, and to paste in the results as proof that the code works.

@steveluscher steveluscher self-requested a review November 19, 2024 17:02
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

giphy

To get your PRs in faster, make sure to test your changes, to write down in your PR description exactly what you did, and to paste in the results as proof that the code works.

Doing this will make it really easy for me to accept your PR with high confidence.

  1. Run code quality scripts

     pnpm test:lint && pnpm test:typecheck && pnpm test:prettier
    
  2. Build the code

     pnpm compile:js && pnpm compile:typedefs
    
  3. Start a REPL

     pnpm node
     > const { Connection } = require('./lib/index.cjs.js');
     > const connection = new Connection('https://api.devnet.solana.com');
    
  4. Do your testing. Use the connection instance to make a query, show that the programId field materializes on the response, and that there are no fatals. Paste all of your steps and results into the PR description.

@@ -2330,6 +2331,7 @@ const TokenBalanceResult = pick({
accountIndex: number(),
mint: string(),
owner: optional(string()),
programId:optional(string()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see that you haven't run any tests or build scripts, because this line of code isn't formatted.

Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Almost!

The data structure you edited is called TokenBalance

https://github.com/solana-labs/solana-web3.js/blob/maintenance/v1.x/src/connection.ts#L1049-L1054

You can see it in use here:

https://github.com/solana-labs/solana-web3.js/blob/maintenance/v1.x/src/connection.ts#L1085-L1088

And here:

https://github.com/solana-labs/solana-web3.js/blob/maintenance/v1.x/src/connection.ts#L1116-L1119

Those properties are materialized here:

https://github.com/solana-labs/solana-web3.js/blob/maintenance/v1.x/src/connection.ts#L2344-L2370

And here:

https://github.com/solana-labs/solana-web3.js/blob/maintenance/v1.x/src/connection.ts#L2375-L2395

Look at all of the RPC calls that materialize ConfirmedTransactionMetaResult and ParsedConfirmedTransactionMetaResult.

You'll see that one such call is this one:

https://github.com/solana-labs/solana-web3.js/blob/maintenance/v1.x/src/connection.ts#L2573C7-L2583

So find out where that is used:

https://github.com/solana-labs/solana-web3.js/blob/maintenance/v1.x/src/connection.ts#L5053-L5083

And there we have an actual RPC call you can make; getTransaction(). There are many others, but this is one. You could conceive of a transaction that you could get, then check result.meta.postTokenBalances to see if you got the new property!

Just make sure you actually use a transaction that involves SPL tokens, or that array will be empty and you won't learn anything.

@shivaji43
Copy link
Contributor Author

Screenshot 2024-11-20 152521

I have used a real Transaction signature of mine for a spl token , and I could see the programId field as a property in the array , am I correct?

@steveluscher
Copy link
Collaborator

Beautiful. Thank you!

@steveluscher steveluscher merged commit 526ce5f into solana-labs:maintenance/v1.x Nov 20, 2024
5 of 6 checks passed
Copy link
Contributor

🎉 This PR is included in version 1.95.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@shivaji43
Copy link
Contributor Author

Thank you so much @steveluscher

Copy link
Contributor

github-actions bot commented Dec 5, 2024

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants