Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Sanity-check balances #1118

Closed
pjz opened this issue Jul 5, 2013 · 13 comments · Fixed by #1768
Closed

Sanity-check balances #1118

pjz opened this issue Jul 5, 2013 · 13 comments · Fixed by #1768

Comments

@pjz
Copy link

pjz commented Jul 5, 2013

participant.balance is more-or-less a cache of the sum of transfers and such. How about after payday have gittip run a self-check by verifying that those two computations match up? To help detect problems like #1116 in the future.

@chadwhitacre
Copy link
Contributor

We should at least do this as a one-off right now for #1116.

@ceboudreaux
Copy link
Contributor

"Receiving: $0.00" in top right corner is not updating. In my history, I see that I am now receiving $.25 from someone, but this is not reflected in the top right corner.

@chadwhitacre
Copy link
Contributor

@ceboudreaux a) That's worth a separate ticket. b) I checked, and that's your take from the Gittip account. It shows up in History because it actually happened. It doesn't show up in "receiving" because there's no cc attached to Gittip and we only include "backed" tips in the future receiving estimate.

I've ticketed revealing when money is from a team as #1126, and making "receiving" more accurate as #1127.

@zbynekwinkler
Copy link
Contributor

This was still somewhat bugging me at the back of my mind so I came up with this:

select username, sum(a) as balance
from (
      select participant as username, sum(amount) as a
        from exchanges 
    group by username 

    union

      select tipper as username, sum(-amount) as a
        from transfers
    group by tipper

    union

      select tippee as username, sum(amount) as a
        from transfers
    group by tippee
) as foo
group by username

except

select username, balance 
from participants ;

The query is designed so it runs through each of the tables only ones (or twice) compared to #1633 (comment) which was timing out due to O(n^2) complexity. Running time 200ms on a local backup (there is a payday in progress in the hot db so it makes no sense to run it there).

Right now it returns 100 rows. I've checked one of them and the balance is off by sum of fees in exchanges. By looking into the code I see that fees are already included in the amount so I believe that the above calculation is correct.

@whit537 Is something broken in our balance computation or in the query above? I am not completely ruling out the possibility that I still do not grok all the details in the current schema. However I thought that I was getting really close 👅

@zbynekwinkler
Copy link
Contributor

#269 (comment)

Ooh! Okay, here's what's going on: fees for payouts aren't included in the exchanges.amount column. Maybe they should be?

Is this what is going on?

@zbynekwinkler
Copy link
Contributor

Changing the query to reflect the above I get only 4 rows (bwt: alexpott is still one of them):

select username, sum(a) as balance
  from (
          select participant as username, sum(amount) as a
            from exchanges 
           where amount > 0
        group by participant

           union

          select participant as username, sum(amount-fee) as a
            from exchanges 
           where amount < 0
        group by participant

           union

          select tipper as username, sum(-amount) as a
            from transfers
        group by tipper

           union

          select tippee as username, sum(amount) as a
            from transfers
        group by tippee
        ) as foo
group by username

except

select username, balance 
from participants 
;

I must say I find it surprising (not the good kind) that the fees are sometimes included in the amount and sometimes not. What is the reasoning behind that decision?

@zbynekwinkler
Copy link
Contributor

It turned out I was using old backup (the one before the fix in #1633). I am happy to report that the above query works and finds no rows in the current database (it found 4 rows before the fix). I believe this query faithfully describes the relation between balances, transfers and exchanges and can be run anytime (today it took just 420ms on heroku db).

@chadwhitacre
Copy link
Contributor

@zwn I've brought this back off the Chopping Block to Infrastructure.

Want to add this to payday before/after payday? Probably another method on the Payday class and two extra calls in .run?

IRC

@chadwhitacre
Copy link
Contributor

I must say I find it surprising (not the good kind) that the fees are sometimes included in the amount and sometimes not. What is the reasoning behind that decision?

I did document this somewhat:

http://gittip.readthedocs.org/en/latest/

@zbynekwinkler
Copy link
Contributor

I'd like to to continue the discussion about what would be the principle of least surprise regarding the exchanges table.

The least surprising would be if both payins and payouts behaved the same. I'd like to be able to do either one of

  • balance = balance + amount - fee
  • balance = balance + amount

not depending on the sign of the amount. It does not really matter which one. The first variant behaves like when we are taking the fee of the balance. The second variant behaves like the user pays the fee before the money reaches gittip or after it has left. Both are fine choices.

This was referenced Nov 30, 2013
@chadwhitacre
Copy link
Contributor

I'd like to to continue the discussion about what would be the principle of least surprise regarding the exchanges table.

Agreed. But:

  1. Does that discussion belong on this ticket or a new one?
  2. Should we postpone that discussion until after Clean up db schema #1549?

@chadwhitacre
Copy link
Contributor

I think to close this ticket we need to add an extra method to the Payday class and call it in .run before/after the rest of the payday method calls.

@chadwhitacre
Copy link
Contributor

Unassigning myself per IRC.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants