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

Add example for remaining_accounts in anchor doc #2350

Closed

Conversation

Purva-Chaudhari
Copy link

Created a section for remaining_accounts in the program-module.md
Added code example of distribution of lamports from PDA to dynamic number of accounts

(Useful to have in doc to highlight the feature of keeping number of accounts variable in solana through remaining_accounts which is otherwise difficult)

Let me know your suggestions on PR (any other place to add the code example if the current one doesn't look apt)

@vercel
Copy link

vercel bot commented Dec 30, 2022

Someone is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Dec 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
anchor-docs ✅ Ready (Inspect) Visit Preview Dec 30, 2022 at 3:28PM (UTC)

@Henry-E
Copy link
Contributor

Henry-E commented Jan 3, 2023

Thanks for the PR!

Honestly though, I'm not sure that remaining accounts is important enough to warrant such a large code example specifically there in the docs. There's a few different things as well in the example that I wouldn't be sure about introducing there; namely the lifetimes seem superfluous and also manually editing lamports is a bit fiddly.

As a compromise, is there some example in the tests/ directory that uses remaining accounts that we could point to as example usage of remaining accounts?

@Purva-Chaudhari
Copy link
Author

I see, just thought to add cause saw quite some issues on discord and solana stack for the same. Can maybe point to multisig one of the multisig examples if thats okay?

@Henry-E
Copy link
Contributor

Henry-E commented Jan 4, 2023

Can you share a link to the multisig example you're thinking of?

@Purva-Chaudhari
Copy link
Author

This one

.remainingAccounts([

(Not explicitly implying the dynamic account part, but the multisig gives the context of the same which could be something to refer)

@Henry-E
Copy link
Contributor

Henry-E commented Jan 5, 2023

hmm yeh, not much of an example in the multisig test tbh. If you wanted to add your program function + typescript test to the /tests/misc that would probably be fine and i wouldn't mind merging that too much.

Minor nitpicks:

  • don't need the lifetimes on the program function (i don't think?)
  • in ts it might be useful to explicitly type the account meta array just so that it's clear to people that you're supposed to construct an array of account meta variables (which have their own specific arguments) in order to pass it into the remaining accounts part.
  • the example is just a bit messy in general, you're finding a PDA address but then you're never actually checking the seed or using it in the program? Plus the Sample account is never actually used.
  • Instead of manually updating the lamports amounts it would be simpler to just use the system program transfer function to transfer lamports from the signer to the target accounts. You would just need to construct the CPI context and call this function exposed by anchor https://github.com/coral-xyz/anchor/blob/master/lang/src/system_program.rs#L298
  • you can use for remaining_account in ctx.remaining_accounts { instead of a while loop with an index
  • calculate the amount_div a single time outside of the loop

Reading this example even more, i'm not even sure the program function compiles or makes sense. It's bad practice to manually move lamports out of a program account that stores data on it. It'll be a lot cleaner once you just switch to transfering from the signing accounts and fix some of the nitpicks.

Thanks!

@Purva-Chaudhari
Copy link
Author

Purva-Chaudhari commented Jan 9, 2023

Hey thanks for the comments -

If you find it would be good to add in the tests section, I can make the changes and would be happy to add a working example there (for token/lamport transfer to dynamic accounts).

Or can simply revise my example and put a blog for that :)

@Henry-E
Copy link
Contributor

Henry-E commented Jan 9, 2023

It's up to yourself but if you cleaned it up a bit and added it as a test to /tests/misc i'd be happy to merge.

@Purva-Chaudhari
Copy link
Author

On it, let me add the test

@Purva-Chaudhari
Copy link
Author

Hi @Henry-E , any contributor guide of anchor? or for unit tests, can i just run the test/misc and push?

@Henry-E
Copy link
Contributor

Henry-E commented Jan 19, 2023

# cargo build anchor-proper
cargo build
# alias anchor to the built package
alias anchor="$(pwd)/target/debug/anchor"
# build the local packages
cd ts/packages/borsh; yarn build
cd ../anchor; yarn build
cd ../../..
# cd into whatever test directory
cd tests/misc
yarn link "@coral-xyz/anchor" && yarn --frozen-lockfile

This is the rough sequence of commands I call to run stuff locally when modifying /lang.

However I don't think your tests rely on any core changes, so I think you can just do anchor test in the tests/misc directory as normal. Also you can check out what the CI does, it also just calls anchor test whenever someone pushes their code. But it would still be best for you to test and debug locally first!

@Purva-Chaudhari
Copy link
Author

let me try , thanks !

@Purva-Chaudhari
Copy link
Author

Test working in private local anchor project.

@Purva-Chaudhari
Copy link
Author

corrected cli error.
cli command passing locally for tests/misc

@Purva-Chaudhari
Copy link
Author

I adjusted the program id to my localnet, How do i make the ci tests pass. Locally its passing in misc folder

@Henry-E
Copy link
Contributor

Henry-E commented Mar 7, 2023

Sorry but i'm really not sure. Maybe let's just leave the PR for now.

@Henry-E Henry-E closed this Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants