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

Typescript complains when .accounts() from MethodNamespace is missing an account #1547

Closed
danmt opened this issue Mar 3, 2022 · 9 comments · Fixed by #1548
Closed

Typescript complains when .accounts() from MethodNamespace is missing an account #1547

danmt opened this issue Mar 3, 2022 · 9 comments · Fixed by #1548

Comments

@danmt
Copy link
Contributor

danmt commented Mar 3, 2022

Description

MethodsNamespace has a mechanism to resolve accounts, with this we can avoid common accounts like SystemProgram, as well as PDAs since they are auto-populated.

Problem

Due to this change, the accounts parameter is no longer any, which is awesome but now Typescript complains when not all the accounts are provided. This makes it impossible to use the account resolver logic.

Proposed Solution

While it isn't pretty, we could wrap the accounts method parameter with Partial. This would bypass the error but makes it possible for developers to make a method call with required accounts missing. I think a better solution would be a mechanism to differentiate mandatory from optional accounts.

NOTE: I'm creating this issue without a PR because I'm not entirely sure which would be the best way to go.

@callensm
Copy link
Member

callensm commented Mar 3, 2022

ah yep, i'll fix this...just needs an Exclude wrapper for the account names for the ones handled by the resolver.

@danmt
Copy link
Contributor Author

danmt commented Mar 3, 2022

I like that solution much better. I'm far from good when it comes to Typescript complex types... As a hotfix for my use case, I did the Partial thing I mentioned and it works but yeah, it's kinda crappy.

My concern with Exclude is that I thought that you could either let the resolver put the accounts for you or you can put it manually. The Exclude might prevent users from manually adding an account, but I'm not sure if that's even an expected use-case.

@callensm
Copy link
Member

callensm commented Mar 3, 2022

My thinking was that if we have an account resolver built in for things like systemProgram, tokenProgram, etc. then we should start discouraging / preventing people from providing it manually to prevent mistakes. In that case, Exclude would be perfect.

@danmt
Copy link
Contributor Author

danmt commented Mar 3, 2022

Totally agree with systemProgram and so on, but I'm not entirely convinced on PDAs, don't know why I gotta feel that someone might need to add a PDA account manually. This isn't my case, but don't know, that's my concern.

@callensm
Copy link
Member

callensm commented Mar 3, 2022

This would be a patch to prevent purely for the global chain programs mentions above from being manually added.

@danmt
Copy link
Contributor Author

danmt commented Mar 3, 2022

That means PDA accounts would still need to be manually added?

@danmt
Copy link
Contributor Author

danmt commented Mar 3, 2022

Looking at this condition it seems like we need to Exclude global chain programs and accounts at PDAs should be optional, so autoPopulate is called only when the account wasn't provided.

@callensm
Copy link
Member

callensm commented Mar 3, 2022

I'm almost done with the PR and will link to this issue to see what my resolution was.

@danmt
Copy link
Contributor Author

danmt commented Mar 3, 2022

Just read the PR and it seems to cover the global chain programs part. But I don't think it solves the issue for accounts at PDAs, the accounts method will still ask for those accounts even though they could be auto-populated.

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 a pull request may close this issue.

2 participants