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

Query all types of balances for an account and denom #504

Merged
merged 16 commits into from
Jun 6, 2023

Conversation

wojtek-coreum
Copy link
Collaborator

@wojtek-coreum wojtek-coreum commented May 22, 2023

This change is Reviewable

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @silverspase, and @ysv)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)


app/app.go line 442 at r1 (raw file):

		wasmkeeper.WithMessageEncoders(wasmcustomhandler.NewCoreumMsgHandler()),
		wasmkeeper.WithQueryPlugins(wasmcustomhandler.NewCoreumQueryHandler(
			assetftkeeper.NewQueryService(app.AssetFTKeeper, app.BankKeeper),

@ysv @miladz68 @wojtek-coreum Do we want to include the Query for the WASM contracts?


x/asset/ft/keeper/grpc_query.go line 93 at r1 (raw file):

		Balance:     qs.bankKeeper.GetBalance(ctx, account, denom).Amount,
		Whitelisted: qs.keeper.GetWhitelistedBalance(ctx, account, denom).Amount,
		Frozen:      qs.keeper.GetFrozenBalance(ctx, account, denom).Amount,

You have defined the Locked in the protos, but here it's empty.

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)


-- commits line 1 at r1:
Why the CLI isn't implemented?


-- commits line 2 at r1:
What about the integration test?

miladz68
miladz68 previously approved these changes May 23, 2023
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)


app/app.go line 442 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv @miladz68 @wojtek-coreum Do we want to include the Query for the WASM contracts?

@dzmitryhil which queries exactly ? let's discuss.

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @silverspase, and @ysv)


-- commits line 1 at r1:

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why the CLI isn't implemented?

It is


-- commits line 2 at r1:

Previously, dzmitryhil (Dzmitry Hil) wrote…

What about the integration test?

They are there


app/app.go line 442 at r1 (raw file):

Previously, miladz68 (milad) wrote…

@dzmitryhil which queries exactly ? let's discuss.

yes I guess


x/asset/ft/keeper/grpc_query.go line 93 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You have defined the Locked in the protos, but here it's empty.

I removed it from protos later

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 14 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)


app/app.go line 442 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

yes I guess

added handler here, later on we must add types to wasm sdk. I also see that we are missing other queries (FrozenBalances and WhitelistedBalances)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 14 files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)


app/app.go line 442 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

added handler here, later on we must add types to wasm sdk. I also see that we are missing other queries (FrozenBalances and WhitelistedBalances)

@ysv Let's add a task to extend the WASM FT test and SDK with that query.


integration-tests/modules/assetft_test.go line 307 at r2 (raw file):

	require.NoError(t, err)

	assertT.Equal("30", resp.Whitelisted.String())

How about move the coins (balance, frozen, whitelisted) to the variables use them in the TXs and assert them at the end instead of strings.

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 14 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)


integration-tests/modules/assetft_test.go line 307 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

How about move the coins (balance, frozen, whitelisted) to the variables use them in the TXs and assert them at the end instead of strings.

Done.

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @silverspase, and @ysv)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @ysv)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 13 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @ysv)

@ysv ysv requested a review from dzmitryhil May 24, 2023 09:53
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 13 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @silverspase, and @wojtek-coreum)


app/app.go line 442 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv Let's add a task to extend the WASM FT test and SDK with that query.

added


proto/coreum/asset/ft/v1/query.proto line 94 at r3 (raw file):

}

message QueryBalanceResponse {

Here is the response structure we agreed to have from clickup ticket:

{
  denom: uft-abcde
  balance: 100,
  whitelisted: 200,
  frozen: 50,
  locked: 10, // this value comes from vesting.
  source: assetft,
}

proto/coreum/asset/ft/v1/query.proto line 94 at r3 (raw file):

}

message QueryBalanceResponse {

does this query return balance for non asset FT denoms ?

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @silverspase, and @ysv)


proto/coreum/asset/ft/v1/query.proto line 94 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

Here is the response structure we agreed to have from clickup ticket:

{
  denom: uft-abcde
  balance: 100,
  whitelisted: 200,
  frozen: 50,
  locked: 10, // this value comes from vesting.
  source: assetft,
}

let's discuss it after daily


proto/coreum/asset/ft/v1/query.proto line 94 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

does this query return balance for non asset FT denoms ?

yes

dzmitryhil
dzmitryhil previously approved these changes May 24, 2023
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase and @ysv)

miladz68
miladz68 previously approved these changes May 26, 2023
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase and @ysv)

@wojtek-coreum wojtek-coreum requested a review from a team as a code owner May 26, 2023 12:21
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @wojtek-coreum)


proto/coreum/asset/ft/v1/query.proto line 94 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

let's discuss it after daily

{
  balance: 100,
  whitelisted: 200,
  frozen: 50,
  locked: 10, // this value comes from vesting.
  source: assetft, // native, ibc, assetft
}

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @wojtek-coreum)


proto/coreum/asset/ft/v1/query.proto line 94 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…
{
  balance: 100,
  whitelisted: 200,
  frozen: 50,
  locked: 10, // this value comes from vesting.
  source: assetft, // native, ibc, assetft
}

final decision:

{
 balance: 100,
 whitelisted: 200,
 frozen: 50,
 locked: 10, // this value comes from vesting.
}

for non-existing denom return 0s

@wojtek-coreum wojtek-coreum dismissed stale reviews from miladz68 and dzmitryhil via a4948d7 May 29, 2023 08:29
Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 15 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)


proto/coreum/asset/ft/v1/query.proto line 94 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

final decision:

{
 balance: 100,
 whitelisted: 200,
 frozen: 50,
 locked: 10, // this value comes from vesting.
}

for non-existing denom return 0s

Done.

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 6 files at r5, 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @silverspase)

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @silverspase)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 6 files at r5, 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @silverspase)

@wojtek-coreum wojtek-coreum merged commit 97b043c into master Jun 6, 2023
@wojtek-coreum wojtek-coreum deleted the wojtek/asset-balance-query branch June 6, 2023 12:32
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