account min-balance #825

tzaffi · 2022-01-14T22:23:49Z

Add `min-balance` information to the `/v2/accounts` endpoint

Fixes

#808 #676

Summary of changes

accounting/calculated.go - EnrichMinBalance: main workhorse of thie PR
accounting/calculated_test.go - new unit tests
api/error_messages.go - new error message
api/handlers.go - mostly in fetchAccounts()
api/indexer.oas2.json - add min-balance and fix a couple of typos (discovered by future circle test in Nightly Tests: compare2algod in addition to the regular tests #839 )
idb/postgres/postgres.go - modify GetAccounts adding a third output of type *bookkeeping.BlockHeader

Minor Changes in `GetAccounts()` Just for Tests to Pass

api/handlers_test.go
cmd/e2equeries/main.go
cmd/idbtest/idbtest.go
idb/dummy/dummy.go
idb/idb.go
idb/mocks/IndexerDb.go
idb/postgres/postgres_integration_test.go
util/test/testutil.go

Auto Generated

api/generated/common/routes.go
api/generated/common/types.go
api/generated/v2/routes.go
api/generated/v2/types.go
api/indexer.oas3.yml

Test Plan

Unit Tests. Working on an integration tests via the sandbox.

Related Follow-up PR

#839 - Add a test that alerts automatically when a new discrepency arises between algod and indexer

`algod` compatriate

algorand/go-algorand#3287

codecov-commenter · 2022-01-14T22:34:32Z

Codecov Report

Merging #825 (aca823c) into develop (bf4571d) will increase coverage by 0.51%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop     #825      +/-   ##
===========================================
+ Coverage    57.67%   58.19%   +0.51%     
===========================================
  Files           35       36       +1     
  Lines         4362     4432      +70     
===========================================
+ Hits          2516     2579      +63     
- Misses        1539     1545       +6     
- Partials       307      308       +1

Impacted Files	Coverage Δ
api/error_messages.go	`100.00% <ø> (ø)`
idb/idb.go	`51.21% <ø> (ø)`
idb/postgres/postgres.go	`49.01% <14.28%> (ø)`
api/handlers.go	`59.59% <50.00%> (-0.25%)`	⬇️
accounting/calculated.go	`95.23% <95.23%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf4571d...aca823c. Read the comment docs.

Makefile

tzaffi · 2022-01-16T05:21:11Z

README.md

@@ -1,23 +1,28 @@
+<!-- markdownlint-disable MD041 -->


linting with vscode's markdownlint extension

tzaffi · 2022-01-16T05:22:06Z

api/README.md

 The Makefile will install our fork of **oapi-codegen**, use `make oapi-codegen` to install it directly.

-1. Document your changes by editing **indexer.oas2.yml**
+1. Document your changes by editing **indexer.oas2.json**


tzaffi · 2022-01-16T05:27:04Z

parity/json_diff.py

@@ -0,0 +1,197 @@
+from copy import deepcopy


Code to generate algod v. indexer parity reports. I include a circle test that:

pulls latest go-algorand submodule

uses this library to generate yaml reports

asserts that the results are as expected. In particular:

all essential features of algod have their counterparts in indexer (think of min-balance)

terminology remain consistent

etc.

tzaffi · 2022-01-16T21:27:29Z

parity/reports/algod2indexer_all_summary.txt

@@ -0,0 +1,115 @@
+____________________      61 New Attributes       ____________________


This report shows all the "essential" differences between algod and indexer in a relatively human-friendly format. There are variants of this report that limit the scope to:

changes of common attributes

attributes which are not present in algod but exist in indexer

attributes which are present in algod but missing from indexer

However, this report shos ALL the differences (the union of the above)

tzaffi · 2022-01-17T15:57:51Z

parity/reports/algod2indexer_full.yml

+      - INDEXER: '"[\\lsch\\] global schema"'
+      - ALGOD: '"[\\gsch\\] global schema"'


tzaffi · 2022-01-17T15:58:13Z

parity/reports/algod2indexer_full.yml

+      - INDEXER: '"\\[tt\\] value type."'
+      - ALGOD: '"\\[tt\\] value type. Value `1` refers to **bytes**, value `2` refers to **uint**"'


looks outdated on indexer

tzaffi · 2022-01-25T05:01:18Z

accounting/calculated.go

+// Some data which is relevant for MinBalance but duplicated is ignored. In particular:
+// * TotalAppSchema - gotten from account.AppsTotalSchema and not re-calc'ed from CreatedApps
+// * TotalExtraPages - gotten from account.AppsTotalExtraPages and not re-calc'ed from CreatedApps
+func minBalanceProjection(account *generated.Account) basics.AccountData {


There is a similar function AccountToAccountData in go-algorand that fills out all of the fields (but has some minor issues when applied to Indexer's generated.Account). It is also possible to try and unmarshall the basics.Account directly from the database as is partially done elsewhere. After some thought, I decided that this was the simplest approach - get only the essential data into a basics.AccountData at the most convenient location and calculate MinBalance from it.

tzaffi · 2022-01-25T05:05:13Z

api/handlers.go

@@ -13,6 +13,7 @@ import (
 	log "github.com/sirupsen/logrus"

 	"github.com/algorand/go-algorand/data/basics"
+	"github.com/algorand/go-algorand/data/bookkeeping"


for BlockHeader

api/handlers.go

tzaffi · 2022-01-25T05:10:31Z

api/indexer.oas2.json

@@ -960,7 +964,7 @@
      ],
      "properties": {
        "type": {
-          "description": "\\[tt\\] value type.",
+          "description": "\\[tt\\] value type. Value `1` refers to **bytes**, value `2` refers to **uint**",


This was spotted by the new python indexer vs. algod parity test.

tzaffi · 2022-01-25T05:10:39Z

api/indexer.oas2.json

@@ -1036,7 +1040,7 @@
          "$ref": "#/definitions/ApplicationStateSchema"
        },
        "global-state-schema": {
-          "description": "[\\lsch\\] global schema",
+          "description": "[\\gsch\\] global schema",


api/handlers_test.go

accounting/calculated.go

tzaffi · 2022-01-25T23:31:28Z

@winder and @algorandskiy have brought to my attention some important changes that are in the works to handle unlimited assets and which affect the way min-balance is calculated (though technically, the formula remains the same). In particular, we can see that new Total* fields will enter the ledger through a brand new struct AccountBaseData. It may make sense for indexer to import these new ledger fields directly, in which case the minbalance calculation would use the new stand alone MinBalance func something like the following:

var a basics.Account // which perhaps? has as part of it a ledger.AccountBase, or something like that

... populate a ...

minBalance := basics.MinBalance(proto, a.TotalAssets, a.TotalAppSchema, a.TotalAppParams, a.TotalAppLocalStates, a.TotalExtraAPpPages)

@winder - Do you think we should wait for these changes to the ledger to get merged into go-algorand before continuing with this PR? On the other hand, do you see any fairly simple and future-aware work-arounds that can let us proceed without waiting?

winder · 2022-01-26T14:21:18Z

idb/idb.go

@@ -252,6 +252,8 @@ type AccountQueryOptions struct {

 	IncludeAssetHoldings bool
 	IncludeAssetParams   bool
+	// IncludeMinBalance indicates whether to calculate min-balance as well (a realatively slow operation)
+	IncludeMinBalance bool


These are database layer options, so this option doesn't need to be here. I think we can get away with using the nil round check, or just skip/unset it in the rewind function.

winder · 2022-01-26T14:41:22Z

idb/idb.go

@@ -173,7 +173,7 @@ type IndexerDb interface {
 	// The next multiple functions return a channel with results as well as the latest round
 	// accounted.
 	Transactions(ctx context.Context, tf TransactionFilter) (<-chan TxnRow, uint64)
-	GetAccounts(ctx context.Context, opts AccountQueryOptions) (<-chan AccountRow, uint64)
+	GetAccounts(ctx context.Context, opts AccountQueryOptions) (<-chan AccountRow, uint64, *bookkeeping.BlockHeader)


@tolikzinovyev I'm curious what you think about this. The protocol is needed for the min balance calculation, but this approach puts accounting logic in the REST API. There are a few other bits of accounting logic outside of the eval function (sig type, created/deleted at, pending rewards calculations, maybe others). Is there a way to make this sort of accounting logic pluggable so that it can be encapsulated so that it doesn't need to be part of the DB/REST API?

cc: @AlgoStephenAkiki @shiqizng

I think the right solution is to return basics.AccountDatas in IndexerDb.GetAccounts() like it's done in other IndexerDb functions, and in api invoke basics.AccountData.MinBalance().

That seems reasonable, but it still has the same problem, you need ConsensusParams to pass into MinBalance.

I wouldn't say it's a problem. GetAccounts() returns the current round. Similarly, it can return the current consensus version.

An Even Quicker Approach

If we're aiming to get this PR out as soon as possible, an even quicker option is C which doesn't return any basics.AccountData. I could still push the calc of MinBalance closer to the view without a large effort. In this case, I would still create issues similar to the above.

I think we need to wait for the ledger refactoring changes. Whether or not these approaches will continue working depend on changes that are yet to be made. For example, do we add the asset counts lazily or as a migration? If it's done lazily we may not always be able to compute the min balance depending on the REST API changes for the ledger refactoring (one proposal includes an option to return account data with asset/application data removed).

Regardless of that, I think we should skip the basics.AccountData conversion. Does it really help solve our problem? We're constructing it from metadata outside of algod, so we may end up with data inconsistencies based on how we reassemble it from Postgres.

Agree with the first part, but ideally the database layer shouldn't construct api types imo. Constructing basics.AccountData from data in postgres is pretty straightforward, it's already done in idb/postgres/internal/ledger_for_evaluator/.

I support Will's position that we wait for the ledger refactoring changes. We don't want to add this now, and then have updates to this work become a blocker for the ledger refactoring project (we have enough complex dependencies there already).
People can continue to work without min balance being easily available for a little bit longer.

…affi/min-balance

markdown lint

3e9c0c3

tzaffi marked this pull request as draft January 14, 2022 22:23

tzaffi linked an issue Jan 14, 2022 that may be closed by this pull request

Expose account info field min-balance #808

Closed

Merge remote-tracking branch 'origin' into tzaffi/min-balance

eadb8a7

Zeph Grunschlag added 7 commits January 15, 2022 09:58

update submodule

1432816

more control ovedr submodule

957070c

typo

5a67bc5

experimental differ scripts

f0da64b

reporting diffs

bbfb7fe

reasonable looking reports

45babc0

human friendly parity report

7f658fe

tzaffi commented Jan 16, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

tzaffi commented Jan 16, 2022

View reviewed changes

Zeph Grunschlag added 4 commits January 15, 2022 23:47

maybe now it's human legible

98205d0

maybe now it human legible

0e0f61f

reporting in good shape

cdd986e

summary code

779a347

tzaffi commented Jan 16, 2022

View reviewed changes

Zeph Grunschlag added 7 commits January 16, 2022 23:56

wip

2851191

rm

bde9d1a

rm

23a7264

yaml is the best

b727bdb

looking good

ceb74ae

bare essentials diff of swaggers

a7335a0

set asserted diffs as drops and full

997de01

tzaffi commented Jan 17, 2022

View reviewed changes

Update Makefile

f8e5b39

tzaffi commented Jan 25, 2022

View reviewed changes

api/handlers.go Outdated Show resolved Hide resolved

oops == nil

7ef690b

tzaffi commented Jan 25, 2022

View reviewed changes

Zeph Grunschlag added 2 commits January 24, 2022 23:18

go fmt

2bc1eb1

lint

e73ec07

tzaffi commented Jan 25, 2022

View reviewed changes

api/handlers_test.go Outdated Show resolved Hide resolved

tzaffi and others added 3 commits January 24, 2022 23:47

Update api/handlers_test.go

b11545f

Merge remote-tracking branch 'origin' into tzaffi/min-balance

fe41ad9

breakout algodvindexer into its own PR

6e8aa8b

tzaffi changed the title ~~WIP: account min-balance~~ account min-balance Jan 25, 2022

re-merge

522a03a

tzaffi marked this pull request as ready for review January 25, 2022 14:44

tzaffi commented Jan 25, 2022

View reviewed changes

accounting/calculated.go Outdated Show resolved Hide resolved

Update accounting/calculated.go

f31b049

tzaffi mentioned this pull request Jan 25, 2022

Nightly Tests: compare2algod in addition to the regular tests #839

Merged

Zeph Grunschlag added 3 commits January 25, 2022 14:57

Merge remote-tracking branch 'origin/develop' into tzaffi/min-balance

2610b8a

merge

e28e1d0

revert

bbd3c66

winder reviewed Jan 26, 2022

View reviewed changes

Zeph Grunschlag added 3 commits January 27, 2022 19:02

Merge branch 'develop' of https://github.com/algorand/indexer into tz…

aca823c

…affi/min-balance

replace entire &blockheader with &blockheader.CurrentProtocol

481e45d

fmt

d45da61

tzaffi closed this Jan 31, 2022

tzaffi deleted the tzaffi/min-balance branch January 31, 2022 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

account min-balance #825

account min-balance #825

tzaffi commented Jan 14, 2022 •

edited

Loading

codecov-commenter commented Jan 14, 2022 •

edited

Loading

tzaffi Jan 16, 2022 •

edited

Loading

tzaffi Jan 16, 2022 •

edited

Loading

tzaffi Jan 16, 2022 •

edited

Loading

tzaffi Jan 16, 2022 •

edited

Loading

tzaffi Jan 17, 2022 •

edited

Loading

tzaffi Jan 17, 2022

tzaffi Jan 25, 2022 •

edited

Loading

tzaffi Jan 25, 2022

tzaffi Jan 25, 2022

tzaffi Jan 25, 2022

tzaffi commented Jan 25, 2022 •

edited

Loading

winder Jan 26, 2022

tzaffi Jan 26, 2022

winder Jan 26, 2022

tolikzinovyev Jan 26, 2022

winder Jan 26, 2022

tolikzinovyev Jan 26, 2022

tzaffi Jan 27, 2022

tzaffi Jan 31, 2022 •

edited

Loading

winder Jan 31, 2022

tolikzinovyev Jan 31, 2022

algoanne Jan 31, 2022

tzaffi Jan 31, 2022

		@@ -0,0 +1,115 @@
		____________________ 61 New Attributes ____________________

		- INDEXER: '"[\\lsch\\] global schema"'
		- ALGOD: '"[\\gsch\\] global schema"'

		- INDEXER: '"\\[tt\\] value type."'
		- ALGOD: '"\\[tt\\] value type. Value `1` refers to bytes, value `2` refers to uint"'

account min-balance #825

account min-balance #825

Conversation

tzaffi commented Jan 14, 2022 • edited Loading

Add min-balance information to the /v2/accounts endpoint

Fixes

Summary of changes

Minor Changes in GetAccounts() Just for Tests to Pass

Auto Generated

Test Plan

Related Follow-up PR

algod compatriate

codecov-commenter commented Jan 14, 2022 • edited Loading

Codecov Report

tzaffi Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

tzaffi Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

tzaffi Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

tzaffi Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

tzaffi Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzaffi Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzaffi commented Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzaffi Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

An Even Quicker Approach

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzaffi commented Jan 14, 2022 •

edited

Loading

Add `min-balance` information to the `/v2/accounts` endpoint

Minor Changes in `GetAccounts()` Just for Tests to Pass

`algod` compatriate

codecov-commenter commented Jan 14, 2022 •

edited

Loading

tzaffi Jan 16, 2022 •

edited

Loading

tzaffi Jan 16, 2022 •

edited

Loading

tzaffi Jan 16, 2022 •

edited

Loading

tzaffi Jan 16, 2022 •

edited

Loading

tzaffi Jan 17, 2022 •

edited

Loading

tzaffi Jan 25, 2022 •

edited

Loading

tzaffi commented Jan 25, 2022 •

edited

Loading

tzaffi Jan 31, 2022 •

edited

Loading