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

I141 #211

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

I141 #211

wants to merge 10 commits into from

Conversation

monyedavid
Copy link
Collaborator

Unconfirmed balance wrong when a payment is made
Resolves issues #177 #141

  - BuildUnconfirmedBalance utility
- BuildUnconfirmedBalanceSpec
- MempoolSpec
   - get confirmed balance (tokens) of address
   - get confirmed Balance (nanoErgs) of address
   - get confirmed balance (tokens) of address
   - get confirmed Balance (nanoErgs) of address
Copy link
Member

@oskin1 oskin1 left a comment

Choose a reason for hiding this comment

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

Same here. Duplicated changes.

@monyedavid monyedavid changed the base branch from master to base_pr May 22, 2022 22:37
@monyedavid monyedavid requested a review from oskin1 May 22, 2022 22:43
@monyedavid
Copy link
Collaborator Author

Same here. Duplicated changes.

@oskin1 retargeted PR to base_pr

@monyedavid monyedavid mentioned this pull request May 22, 2022
# Conflicts:
#	modules/explorer-api/src/main/scala/org/ergoplatform/explorer/http/api/v1/services/Mempool.scala
#	modules/explorer-api/src/test/scala/org/ergoplatform/explorer/v1/services/AddressesSpec.scala
#	modules/explorer-api/src/test/scala/org/ergoplatform/explorer/v1/services/MempoolSpec.scala
Comment on lines 69 to 73
def totalBalanceOf_(address: Address): F[TotalBalance] =
for {
confirmed <- confirmedBalanceOf(address, 0)
unconfirmed <- memprops.getUnconfirmedBalanceByAddress(address, confirmed)
} yield TotalBalance(confirmed, unconfirmed)
Copy link
Member

Choose a reason for hiding this comment

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

It seems this method does the same thing as totalBalanceOf but in a less efficient way. memprops.getUnconfirmedBalanceByAddress operates on the same data from database as

        offChainBalance <- uOutputRepo.sumUnspentByErgoTree(tree)
        offChainAssets  <- uAssetRepo.aggregateUnspentByErgoTree(tree)

The difference only is that old code aggregates balances inside DB and you does all this in the application's runtime.
I believe this doesn't solve the problem.

Copy link
Collaborator Author

@monyedavid monyedavid May 29, 2022

Choose a reason for hiding this comment

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

@oskin1 this totalBalanceOf_ uses memprops.getUnconfirmedBalanceByAddress
which generates unconfirmed balance with reference to:

  • (of new unconfirmed outputs in mempool)
  • (confirmed outputs spent in mempool)
  • and already calculated confirmed balance from confirmedBalanceOf(address, 0)

the old is not inclusive of mempool transactions
so memprops.getUnconfirmedBalanceByAddress(address, confirmed) using all that data; if you have 10Ergs, send 2Erg and received 3Erg your unconfirmed balance should be 11Erg
the old one doesn't return this expected value.

Copy link
Member

Choose a reason for hiding this comment

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

Oh,got it. The old one includes outputs from unconfirmed transactions, so all you need to do is to sum unconfirmed inputs and subtract them from total unconfirmed balance.

Copy link
Collaborator Author

@monyedavid monyedavid May 30, 2022

Choose a reason for hiding this comment

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

thats what I did the first time, it didn't work in cases when an address sends out & receives tokens during the same "confirmation period"

the algorithm to correctly get the new unconfirmed balance works something along the lines of:
foldLeft transactions(items => item(Tx)):
- determine if transaction is a debit/credit via inputErgoTree(source of funds) ☑️
- determine useable outputs:
- output.ergoTree != wallet.ergoTree || for debit transactions
- output.ergoTree == wallet.ergoTree || for credit transactions

defined in this utility:
BuildUnconfirmedBalance

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate why simple aggregation of unconfirmed inputs/outputs doesn't work?
You get the same set of inputs/outputs inside unconfirmed transactions, so the result should be the same, right?

@monyedavid monyedavid requested a review from oskin1 June 7, 2022 06:25
@monyedavid monyedavid marked this pull request as draft June 9, 2022 11:49
Base automatically changed from base_pr to master June 9, 2022 14:08
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.

2 participants