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.

Sign up for GitHub

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

Jump to bottom

Problem: Missing traceable usable balance for account projection #666

Closed
wants to merge 19 commits into from
Closed

Problem: Missing traceable usable balance for account projection #666

wants to merge 19 commits into from

Conversation

davcrypto
Copy link
Contributor

No description provided.

@davcrypto davcrypto marked this pull request as draft January 17, 2022 04:18
@davcrypto davcrypto marked this pull request as ready for review January 17, 2022 04:33
ysong42
ysong42 reviewed Jan 18, 2022
View reviewed changes
Copy link
Contributor

@ysong42 ysong42 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Overall LGTM! The logic is super clear!

usecase/parser/test/end_block_ethereum_send_to_cosmos_handled.go Show resolved Hide resolved
usecase/model/blockresults.go Outdated Show resolved Hide resolved
usecase/event/coin_burn.go Show resolved Hide resolved
projection/account/account.go Outdated Show resolved Hide resolved
projection/account/account.go Outdated Show resolved Hide resolved
projection/account/migrations/20211130143810_usable_balance.down.sql Show resolved Hide resolved
projection/account/view/accounts.go Show resolved Hide resolved
projection/account/account.go Show resolved Hide resolved
davcrypto and others added 4 commits January 18, 2022 16:30
@davcrypto
Merge master into accumulate_balance
e6a1337
@davcrypto @ysong42
Update usecase/model/blockresults.go
b741b37 
Co-authored-by: Yang SONG <86600130+ysong42@users.noreply.github.com>
@davcrypto @ysong42
Update projection/account/account.go
db75361 
Co-authored-by: Yang SONG <86600130+ysong42@users.noreply.github.com>
@davcrypto
DEL unnecessary
6ee98cd
calvinaco
calvinaco reviewed Jan 19, 2022
View reviewed changes
Copy link
Collaborator

@calvinaco calvinaco left a comment

Choose a reason for hiding this comment

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

Leaving some quick comments on the account projection.

Will review other parts in details.

example/config/config.toml Outdated Show resolved Hide resolved
example/app/example-app/projections.go Outdated Show resolved Hide resolved
projection/account/account.go Show resolved Hide resolved
projection/account/account.go
return recipienterr
}
for _, account := range event.Genesis.AppState.Bank.Balances {
for _, accountBalance := range account.Coins {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to cater for vesting balance here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For example using Crypto.org Chain mainnet genesis, you can see some of the account has DelayedVesting and the balance will appear at Gensis.AppState.Bank.Balances

https://mainnet.crypto.org:26657/genesis

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 think Genesis.AppState.Bank.Balances would be enough since catering DelayedVesting would result in double counted

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for the DelayedVesting account, when it reached the vesting date, there will be coin* event for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If those Coin is under DelayedVesting, should we add them as UsableBalance 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.

Right, let me put some effort on it.

davcrypto and others added 3 commits January 20, 2022 14:18
@davcrypto @calvinaco
Update example/app/example-app/projections.go
5f5bb9e 
Co-authored-by: Calvin Lau <38898718+calvinaco@users.noreply.github.com>
@davcrypto
DEL unnecessary
8dc281f
@davcrypto
UPDATE tests
c962376
ysong42
ysong42 approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@ysong42 ysong42 left a comment

Choose a reason for hiding this comment

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

LGTM!

calvinaco
calvinaco reviewed Jan 28, 2022
View reviewed changes
Copy link
Collaborator

@calvinaco calvinaco left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Left 2 comments to follow-up the previously raised issues.

projection/account/account.go Show resolved Hide resolved
projection/account/account.go
return recipienterr
}
for _, account := range event.Genesis.AppState.Bank.Balances {
for _, accountBalance := range account.Coins {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If those Coin is under DelayedVesting, should we add them as UsableBalance here?

@calvinaco
Copy link
Collaborator

Hello @davcrypto , I was looking for something else and come across the CHANGELOG that the coin tracking events (coin_spent, coin_received etc) are introduced at Cosmos SDK v0.43 (Ref: https://github.com/cosmos/cosmos-sdk/blob/master/CHANGELOG.md#state-machine-breaking-2)

As a result, some old data on chain may not have those events actually, not sure if it will break your projection design?

@davcrypto
Copy link
Contributor Author

Thanks @calvinaco and your concern is correct. If events only existed after v0.43, blocks before that would break our assuming and result in a invalid value. We need to use Msg to index that if before v0.43 then.

@davcrypto davcrypto marked this pull request as draft February 7, 2022 10:14
@davcrypto
Copy link
Contributor Author

mentioned solution in #383 for later implementation

@davcrypto davcrypto closed this Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calvinaco calvinaco calvinaco left review comments

@ysong42 ysong42 ysong42 approved these changes

@vincentysc vincentysc Awaiting requested review from vincentysc vincentysc will be requested when the pull request is marked ready for review vincentysc is a code owner

@WilliamXieCrypto WilliamXieCrypto Awaiting requested review from WilliamXieCrypto WilliamXieCrypto will be requested when the pull request is marked ready for review WilliamXieCrypto is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davcrypto @calvinaco @ysong42