Skip to content

Conversation

@awrichar
Copy link
Contributor

@awrichar awrichar commented Nov 1, 2021

Part of #218.

The thing that has up to now been known as an "account" is really just a "balance" - ie given an identity, a pool, and optionally a token index, it tracks a number stating how many of that token are owned. All of this existing functionality has therefore been renamed from "token accounts" to "token balances".

This PR also introduces a new concept of what is a "token account" - currently just a filtered, distinct list of all the identities tracked in "token balances". Eventually this may evolve to include more aggregate details on each identity's holdings, but this is the simplest start without adding another table.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Rather than separate "from" and "to" operations, this should be a single operation
for the database plugin (since it must happen transactionally).

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
This isn't a full-fledged view of accounts, but it provides a way to query
all unique addresses that have (or have had) a non-zero token balance.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2021

Codecov Report

Merging #311 (7b0f034) into main (2bd7e2b) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 7b0f034 differs from pull request most recent head 0b9ca0a. Consider uploading reports for the commit 0b9ca0a to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main      #311   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          229       230    +1     
  Lines        12562     12582   +20     
=========================================
+ Hits         12562     12582   +20     
Impacted Files Coverage Δ
internal/apiserver/route_get_token_accounts.go 100.00% <ø> (ø)
internal/apiserver/route_get_token_pool_by_name.go 100.00% <ø> (ø)
...nternal/apiserver/route_get_token_pools_by_type.go 100.00% <ø> (ø)
...nal/apiserver/route_get_token_transfers_by_pool.go 100.00% <ø> (ø)
...nternal/apiserver/route_post_token_burn_by_type.go 100.00% <ø> (ø)
...nternal/apiserver/route_post_token_mint_by_type.go 100.00% <ø> (ø)
...nternal/apiserver/route_post_token_pool_by_type.go 100.00% <ø> (ø)
...nal/apiserver/route_post_token_transfer_by_type.go 100.00% <ø> (ø)
...rnal/apiserver/route_get_token_accounts_by_pool.go 100.00% <100.00%> (ø)
internal/apiserver/route_get_token_balances.go 100.00% <100.00%> (ø)
... and 7 more

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 2bd7e2b...0b9ca0a. Read the comment docs.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Looks great.

I note even thinking back to the comment above, we’re not committing NoSQL to implement this using distinct. Just putting it into the framework for this implementation 👍

sel = sel.Limit(fi.Limit)
}
}
if fi.Distinct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can’t think of any reason this will be problematic for NoSQL in the future. But an observation that we’re putting another requirement on the truck here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know what, I actually don't have any need to expose this here. I thought I would, but as it played out, this is only used in the database layer (where it has access to the full SelectBuilder anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this requirement from the "Filter" interface. It's now only confined to the SQL code.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
This reverts commit 7ae1afa.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@awrichar awrichar merged commit 57d5d97 into hyperledger:main Nov 3, 2021
@awrichar awrichar deleted the balances branch November 3, 2021 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants