-
Notifications
You must be signed in to change notification settings - Fork 244
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 account balance to account state (#298) #316
Conversation
src/account.ts
Outdated
* Returns calculated account balance | ||
* @returns {Promise<AccountBalance>} | ||
*/ | ||
private async getAccountBalance(): Promise<AccountBalance> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to make this private method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to add it to the state to avoid calling getAccountBalance
in Wallet separately.
src/account.ts
Outdated
@@ -70,6 +80,7 @@ export class Account { | |||
*/ | |||
async state(): Promise<AccountState> { | |||
await this.ready; | |||
this._state.balance = await this.getAccountBalance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not do this, state()
is called in many places, better avoid slowing it down by making additional query every time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes are done.
It might be a bit more efficient if we add optional param to state()
that will decide if the balance should be calculated and added to _state
. With current changes, we need to call state()
again in getAccountBalance
to calculate balance, so for example, in Wallet we will call state()
then call getAccountBalance
that will again call state()
. What do you think?
src/providers/json-rpc-provider.ts
Outdated
* Gets EXPERIMENTAL_genesis_config from RPC | ||
* @returns {Promise<GenesisConfig>} | ||
*/ | ||
async genesisConfig(): Promise<GenesisConfig> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call this experimental_genesisConfig
to indicate that API isn't final and can be changed
src/account.ts
Outdated
@@ -36,6 +36,16 @@ export interface AccountState { | |||
amount: string; | |||
staked: string; | |||
code_hash: string; | |||
storage_usage: number; | |||
locked: string; | |||
balance: AccountBalance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have both locked
and staked
here? What does balance
refer to? how is it different from amount
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have both locked and staked here?
we use locked
to calculate total balance (amount
+ locked
), staked
is not used, it looks like both staked
and accountId
are not fetched from RPC, and can be removed.
What does balance refer to? how is it different from amount?
balance
contains calculated: total balance, state staked, staked and available balance. amount
is fetched from RPC and it's used to calculate total balance.
No description provided.