Fix DEX freeze bug

[masihyeganeh]

Description

Reviewers checklist:

• Try to write more meaningful comments with clear actions to be taken.
• Nit-picking should be unblocking. Focus on core issues.

Authors checklist

• Provide a concise and meaningful description
• Review the code yourself first, before making the PR.
• Annotate your PR in places that require explanation.
• Think and try to split the PR to smaller PR if it is big.

---

This change is 

[ysv]

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

---

integration-tests/modules/dex_test.go line 3058 at r1 (raw file):

// via raw bank keeper. This test asserts the fix: the second order (which would
// lock frozen tokens) must be rejected.
func TestFrozenTokenEscapeViaDEX(t *testing.T) {

are these 2 tests from immunify bugreport ?

they are almost equal so I would keep 1 one of them. Either integration or keeper test

---

x/asset/ft/keeper/keeper_dex.go line 69 at r1 (raw file):

	for _, send := range actions.Send {
		// Validate frozen balances before sending to prevent transferring frozen tokens

does this part of code run during order execution ?

As far as I remember our plan was to make sure that if order is inside orderbook it will be guaranteed executable when matched. And I think this check brakes such logic

Lets discuss

---

integration-tests/modules/dex_test.go line 3230 at r1 (raw file):

		placeOrder2,
	)
	requireT.Error(err, "Second order of 400,000 must fail: it would lock frozen tokens; available spendable is 0")

maybe it makes sense to check that address is not able to lock 100k or 10k also.
Smaller portion of frozen/locked.

[masihyeganeh]

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

---

integration-tests/modules/dex_test.go line 3058 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

are these 2 tests from immunify bugreport ?

they are almost equal so I would keep 1 one of them. Either integration or keeper test

Yes. kept the integration test only

---

integration-tests/modules/dex_test.go line 3230 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

maybe it makes sense to check that address is not able to lock 100k or 10k also.
Smaller portion of frozen/locked.

Done.

---

x/asset/ft/keeper/keeper_dex.go line 69 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

does this part of code run during order execution ?

As far as I remember our plan was to make sure that if order is inside orderbook it will be guaranteed executable when matched. And I think this check brakes such logic

Lets discuss

It was an extra safeguard for a defense in depth. I removed it and still both tests are passing. I also checked the original test and it failed

[miladz68]

@miladz68 made 1 comment.
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on masihyeganeh, metalarm10, TxCorpi0x, and ysv).

---

x/asset/ft/keeper/keeper_dex.go line 670 at r2 (raw file):

	// validate that we don't use frozen coins (if freezing feature is enabled)
	def, err := k.getDefinitionOrNil(ctx, coin.Denom)

Do I understand correctly that we check that frozen is not violated after a dex match happens ?

[ysv]

@ysv made 2 comments and resolved 3 discussions.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on masihyeganeh, metalarm10, miladz68, and TxCorpi0x).

---

x/asset/ft/keeper/keeper_dex.go line 670 at r2 (raw file):

	// validate that we don't use frozen coins (if freezing feature is enabled)
	def, err := k.getDefinitionOrNil(ctx, coin.Denom)

@miladz68 @masihyeganeh
lets double check and make sure this is called ONLY on order placement step not inside matching

---

integration-tests/modules/dex_test.go line 3058 at r2 (raw file):

// via raw bank keeper. This test asserts the fix: the second order (which would
// lock frozen tokens) must be rejected.
func TestFrozenTokenEscapeViaDEX(t *testing.T) {

lets also add intgr or unit-test for the following logic:

1. user has 100TKN availble
2. user creates order for 80TKN sell
3. issuer freezes 60/100 TKN
4. DEX order matched
5. Order should still be executed

(number are subject to change)

[masihyeganeh]

@masihyeganeh made 1 comment.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on metalarm10, miladz68, TxCorpi0x, and ysv).

---

integration-tests/modules/dex_test.go line 3058 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

lets also add intgr or unit-test for the following logic:

1. user has 100TKN availble
2. user creates order for 80TKN sell
3. issuer freezes 60/100 TKN
4. DEX order matched
5. Order should still be executed

(number are subject to change)

Done.

[miladz68]

@miladz68 made 1 comment.
Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on masihyeganeh, metalarm10, TxCorpi0x, and ysv).

---

x/asset/ft/keeper/keeper.go line 1017 at r3 (raw file):

	}

	// Spendable = balance - dexLocked - bankLocked - frozen (additive when freezing enabled)

should we check the spendable balance query ?

[ysv]

@ysv reviewed 5 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on masihyeganeh, metalarm10, and TxCorpi0x).

---

integration-tests/modules/dex_test.go line 3306 at r4 (raw file):

	requireT.NoError(err)

	// Setup: token with freezing, Alice has 100 TKN (1,000,000), Bob has quote

nit: maybe to match with comments we operate with millions of tokens

100 TKN = 100_000_000
80 TKN = 80_000_000

just easier to read

[miladz68]

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