@masihyeganeh reviewed 8 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68, @TxCorpi0x, and @ysv).

@ysv reviewed 8 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on metalarm10, miladz68, and TxCorpi0x).

x/dex/keeper/keeper.go line 433 at r1 (raw file):

				totalReserve = totalReserve.Add(orderData.Reserve)
			}
		}

One case which is not handled here is OrderReserve denom change.
Because reserve is defined in params as Coin and both amount & denom might change.

This is unlikely to be changed, so maybe it is overkill to handle it.
My suggested options are:

1. leave it as it is (ignore denom check) but then we might sum different denom amounts
2. add simple check of denom match and if it is different return error in this query
3. in API return list of reservers - but I don't like idea of making API more complex

What do you think we should do ?

Code quote:

		if orderData.Reserve.IsPositive() {
			if totalReserve.Denom == "" {
				totalReserve = orderData.Reserve
			} else {
				totalReserve = totalReserve.Add(orderData.Reserve)
			}
		}

@metalarm10 made 1 comment.
Reviewable status: 2 of 8 files reviewed, 1 unresolved discussion (waiting on masihyeganeh, miladz68, TxCorpi0x, and ysv).

x/dex/keeper/keeper.go line 433 at r1 (raw file):

In order to prevent panic in this scenario due to sdk.Coin.Add denom incompatibility, I think option-2 is the most viable solution.
We should keep in mind that if the reserve denom ever changes via governance, this query would need to be updated to support multi-denom aggregation.
Fixed: 46d5077

@miladz68 reviewed 8 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on metalarm10, TxCorpi0x, and ysv).

x/dex/keeper/keeper_test.go line 1316 at r2 (raw file):

}

func TestKeeper_GetAccountDEXReserve(t *testing.T) {

you should also add one check into integration tests, it does not have to be a new test, may be an additional check into an existing test.

proto/coreum/dex/v1/query.proto line 173 at r2 (raw file):

// QueryAccountDEXReserveResponse defines the response type for the `AccountDEXReserve` query.
message QueryAccountDEXReserveResponse {

should this query be paginated ? I think users are allowed to create infinite orders.

@ysv reviewed 6 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on miladz68 and TxCorpi0x).

proto/coreum/dex/v1/query.proto line 173 at r2 (raw file):

but how can we make this pagination if it returns total amount ?
your concern is reasonable IMO but I'm not sure we can do paginaiton.

What do you think is the proper way of handling this in comos sdk ?
Should we limit order get query to some number and error if user has more or just ignore it and let node runner limit it by available VM resources ? Or smth else ?

x/dex/keeper/keeper_test.go line 1316 at r2 (raw file):

agree

@metalarm10 made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on miladz68 and TxCorpi0x).

proto/coreum/dex/v1/query.proto line 173 at r2 (raw file):

I think pagination doesn't apply here since we return a single aggregated value, not a list.
Also from readme: The number of active orders a user can have for each denom is limited by a value called max_orders_per_denom, which is determined by DEX governance. The default value is 100.

@metalarm10 made 1 comment.
Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on miladz68, TxCorpi0x, and ysv).

x/dex/keeper/keeper_test.go line 1316 at r2 (raw file):

Done.

@ysv reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on masihyeganeh, miladz68, and TxCorpi0x).

@miladz68 reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on masihyeganeh and TxCorpi0x).