ts: add getAccountInfo helper method to account namespace/client#1084
Conversation
| return await pubkeyUtil.associated(this._programId, ...args); | ||
| } | ||
|
|
||
| async getAccountInfo(address: Address, commitment?: Commitment) { |
There was a problem hiding this comment.
I think this async can be removed?
There was a problem hiding this comment.
Interesting, connection.getAccountInfo return Promise, but no await inside the function.
Q: function return promise or result? 🙂
A: promise will be resolved with async or without async. But for clarity better leave async with await.
There was a problem hiding this comment.
yea you dont need async if you just return sth and theres no await inside the function. if it returns a promise the caller can await. I get the clarity argument but wouldnt it be more idiomatic to have an explicit return type then instead of inference and async which implies there is an await.
There was a problem hiding this comment.
cool happy to amend that, wasn't sure what the convention was for this repo.
Would it make sense to add an eslint rule to enforce it in the future if that's the case?
There was a problem hiding this comment.
my answer was a question. not sure what the convention is myself. dont think we have one except "make prettier happy" for now. but I think that can be a discussion for a different issue
There was a problem hiding this comment.
I'd prefer to have async getAccountInfo with inner await.
There was a problem hiding this comment.
If it helps, there are subtle differences to return Vs return await besides just syntactic ones.
https://eslint.org/docs/rules/no-return-await
p.s. this just has a good description of the subtle differences, not saying I support this particular rule.
https://jakearchibald.com/2017/await-vs-return-vs-return-await/
Don't mind either way, but something to take into consideration perhaps? I think another difference is that the V8 engine provides better stack traces with return await
There was a problem hiding this comment.
I think another difference is that the V8 engine provides better stack traces with return await
so it would seem!
goldbergyoni/nodebestpractices#737
let's do return await then
| const accountInfo = await this._provider.connection.getAccountInfo( | ||
| translateAddress(address) | ||
| ); | ||
| const accountInfo = await this.getAccountInfo(translateAddress(address)); |
There was a problem hiding this comment.
your getAccountInfo translates the address inside so no need to do it here
| TransactionInstruction, | ||
| Commitment, | ||
| GetProgramAccountsFilter, | ||
| AccountInfo, |
There was a problem hiding this comment.
please add a line about the feature to the changelog as well
dominictwlee
changed the title
dominictwlee
changed the title
dominictwlee
force-pushed
the
ts/add-get-account-info-helper
branch
from
f32ca82 to
03fd1ab
Compare
3 participants
Closes #640
This exposes
connection.getAccountInfoto the namespace client's public API.I've written an integration test separately but wasn't sure how to symlink a locally built anchor cli to run the tests. It seems all the integration tests run against the published/installed version of anchor. Should I just do that separately when this change is merged and published?