[1834]: Pay attention to the newly available transfer agent in the se…

…nd restriction. Move the deposit check to SendRestrictionFn (from validateSendDenom) since don't need to repeat that check for every denom. Add a check for withdraw on the from addr since that might now be a marker account. Tweak a couple error messages to distinguish them from eachother. In the transfer endpoint, if the destination is also a marker, check for deposit access on that marker.

x/marker/keeper/keeper_test.go

@@ -429,7 +429,7 @@ func TestAccountKeeperMintBurnCoins(t *testing.T) {
 	require.Error(t, app.MarkerKeeper.DeleteMarker(ctx, user, "testcoin"))
 
 	// Remove escrow balance from account
-	require.NoError(t, app.BankKeeper.SendCoinsFromAccountToModule(ctx, addr, "mint", sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.OneInt()))), "sending coins to module")
+	require.NoError(t, app.BankKeeper.SendCoinsFromAccountToModule(types.WithBypass(ctx), addr, "mint", sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.OneInt()))), "sending coins to module")
 
 	// Succeeds because the bond denom coin was removed.
 	require.NoError(t, app.MarkerKeeper.DeleteMarker(ctx, user, "testcoin"))

x/marker/keeper/marker.go

@@ -640,12 +640,22 @@ func (k Keeper) TransferCoin(ctx sdk.Context, from, to, admin sdk.AccAddress, am
 	if err != nil {
 		return fmt.Errorf("marker not found for %s: %w", amount.Denom, err)
 	}
+
 	if m.GetMarkerType() != types.MarkerType_RestrictedCoin {
 		return fmt.Errorf("marker type is not restricted_coin, brokered transfer not supported")
 	}
+
 	if !m.AddressHasAccess(admin, types.Access_Transfer) {
 		return fmt.Errorf("%s is not allowed to broker transfers", admin.String())
 	}
+
+	// If going to a restricted marker, the admin must have deposit access on that marker too.
+	if toMarker, _ := k.GetMarker(ctx, to); toMarker != nil && toMarker.GetMarkerType() == types.MarkerType_RestrictedCoin {
+		if !toMarker.AddressHasAccess(admin, types.Access_Deposit) {
+			return fmt.Errorf("%s does not have deposit access on %s (%s)", admin, to, toMarker.GetDenom())
+		}
+	}
+
 	if !admin.Equals(from) {
 		switch {
 		case !m.AllowsForcedTransfer():
@@ -657,9 +667,11 @@ func (k Keeper) TransferCoin(ctx sdk.Context, from, to, admin sdk.AccAddress, am
 			return fmt.Errorf("funds are not allowed to be removed from %s", from)
 		}
 	}
+
 	if k.bankKeeper.BlockedAddr(to) {
 		return fmt.Errorf("%s is not allowed to receive funds", to)
 	}
+
 	// set context to having access to bypass attribute restriction test
 	// send the coins between accounts (does not check send_enabled on coin denom)
 	if err = k.bankKeeper.SendCoins(types.WithBypass(ctx), from, to, sdk.NewCoins(amount)); err != nil {

x/marker/keeper/send_restrictions.go

@@ -22,8 +22,48 @@ func (k Keeper) SendRestrictionFn(ctx sdk.Context, fromAddr, toAddr sdk.AccAddre
 		return toAddr, nil
 	}
 
+	// If it's coming from a marker, make sure the withdraw is allowed.
+	admin := types.GetTransferAgent(ctx)
+	if fromMarker, _ := k.GetMarker(ctx, fromAddr); fromMarker != nil {
+		// It shouldn't be possible for the fromAddr to be a marker without a transfer
+		// agent because no keys exist for any marker accounts (so it can't sign a Tx).
+		// So, to be on the safe side, we return an error if that's the case.
+		if len(admin) == 0 {
+			return nil, fmt.Errorf("cannot withdraw from marker account %s (%s)",
+				fromAddr.String(), fromMarker.GetDenom())
+		}
+
+		// That transfer agent must have withdraw access.
+		if !fromMarker.AddressHasAccess(admin, types.Access_Withdraw) {
+			return nil, fmt.Errorf("%s does not have withdraw access for %s (%s)",
+				admin.String(), fromAddr.String(), fromMarker.GetDenom())
+		}
+
+		// Check to see if marker is active; the coins created by a marker can only be withdrawn when it is active.
+		// Any other coins that may be present (collateralized assets?) can be transferred.
+		if fromMarker.GetStatus() != types.StatusActive && !amt.AmountOf(fromMarker.GetDenom()).IsZero() {
+			return nil, fmt.Errorf("cannot withdraw marker created coins (%s) from marker %s (%s) that is not in active status (%s)",
+				amt, fromAddr, fromMarker.GetDenom(), fromMarker.GetStatus().String())
+		}
+	}
+
+	// If it's going to a restricted marker, either the admin (if there is one) or
+	// fromAddr (if there isn't an admin) must have deposit access on that marker.
+	toMarker, _ := k.GetMarker(ctx, toAddr)
+	if toMarker != nil && toMarker.GetMarkerType() == types.MarkerType_RestrictedCoin {
+		addr := admin
+		if len(addr) == 0 {
+			addr = fromAddr
+		}
+		if !toMarker.AddressHasAccess(addr, types.Access_Deposit) {
+			return nil, fmt.Errorf("%s does not have deposit access for %s (%s)",
+				addr.String(), toAddr.String(), toMarker.GetDenom())
+		}
+	}
+
+	// Check the ability to send each denom involved.
 	for _, coin := range amt {
-		if err := k.validateSendDenom(ctx, fromAddr, toAddr, coin.Denom); err != nil {
+		if err := k.validateSendDenom(ctx, fromAddr, toAddr, admin, coin.Denom, toMarker); err != nil {
 			return nil, err
 		}
 	}
@@ -33,19 +73,7 @@ func (k Keeper) SendRestrictionFn(ctx sdk.Context, fromAddr, toAddr sdk.AccAddre
 
 // validateSendDenom makes sure a send of the given denom is allowed for the given addresses.
 // This is NOT the validation that is needed for the marker Transfer endpoint.
-func (k Keeper) validateSendDenom(ctx sdk.Context, fromAddr, toAddr sdk.AccAddress, denom string) error {
-	// If it's going to a restricted marker, fromAddr must have deposit access on that marker.
-	var toMarker types.MarkerAccountI
-	if toAcct := k.authKeeper.GetAccount(ctx, toAddr); toAcct != nil {
-		toAcctAsMarker, isMarker := toAcct.(types.MarkerAccountI)
-		if isMarker {
-			toMarker = toAcctAsMarker
-		}
-	}
-	if toMarker != nil && toMarker.GetMarkerType() == types.MarkerType_RestrictedCoin && !toMarker.AddressHasAccess(fromAddr, types.Access_Deposit) {
-		return fmt.Errorf("%s does not have deposit access for %s (%s)", fromAddr.String(), toAddr.String(), toMarker.GetDenom())
-	}
-
+func (k Keeper) validateSendDenom(ctx sdk.Context, fromAddr, toAddr, admin sdk.AccAddress, denom string, toMarker types.MarkerAccountI) error {
 	markerAddr := types.MustGetMarkerAddress(denom)
 	marker, err := k.GetMarker(ctx, markerAddr)
 	if err != nil {
@@ -57,7 +85,15 @@ func (k Keeper) validateSendDenom(ctx sdk.Context, fromAddr, toAddr sdk.AccAddre
 		return nil
 	}
 
-	// If from address is in deny list, prevent sending of restricted marker
+	// If there's an admin that has transfer access, it's not a normal bank send and there's nothing more to do here.
+	if len(admin) > 0 && marker.AddressHasAccess(admin, types.Access_Transfer) {
+		return nil
+	}
+
+	// If from address is in the deny list, prevent sending of restricted marker.
+	// If the fromAddr is both on the send-deny list and has transfer access, we want to deny this send.
+	// They can either take themselves off the list and do the send again, or just use the transfer endpoint.
+	// But for normal sends (without a transfer agent), we want the send-deny list enforced first.
 	if k.IsSendDeny(ctx, markerAddr, fromAddr) {
 		return fmt.Errorf("%s is on deny list for sending restricted marker", fromAddr.String())
 	}
@@ -67,24 +103,23 @@ func (k Keeper) validateSendDenom(ctx sdk.Context, fromAddr, toAddr sdk.AccAddre
 		return nil
 	}
 
-	reqAttr := marker.GetRequiredAttributes()
-
 	// If going to a marker, transfer permission is required regardless of whether it's coming from a bypass.
 	// If someone wants to deposit funds from a bypass account, they can either send the funds to a valid
 	// intermediary account and deposit them from there, or give the bypass account deposit and transfer permissions.
 	// It's assumed that a marker address cannot be in the bypass list.
 	if toMarker != nil {
-		return fmt.Errorf("%s does not have transfer permissions", fromAddr.String())
+		return fmt.Errorf("%s does not have transfer access for %s", fromAddr.String(), denom)
 	}
 
 	// If there aren't any required attributes, transfer permission is required unless coming from a bypass account.
-	// It's assumed that the only way the restricted coins without required attributes can got into a bypass
+	// It's assumed that the only way the restricted coins without required attributes can get into a bypass
 	// account is by someone with transfer permission, which is then conveyed for this transfer too.
+	reqAttr := marker.GetRequiredAttributes()
 	if len(reqAttr) == 0 {
 		if k.IsReqAttrBypassAddr(fromAddr) {
 			return nil
 		}
-		return fmt.Errorf("%s does not have transfer permissions", fromAddr.String())
+		return fmt.Errorf("%s does not have transfer permissions for %s", fromAddr.String(), denom)
 	}
 
 	// At this point, we know there are required attributes and that fromAddr does not have transfer permission.

x/marker/keeper/send_restrictions_test.go

@@ -162,7 +162,7 @@ func TestSendRestrictionFn(t *testing.T) {
 			from:   owner,
 			to:     addrWithAttrs,
 			amt:    cz(c(1, rDenomNoAttr)),
-			expErr: fmt.Sprintf("%s does not have transfer permissions", owner.String()),
+			expErr: fmt.Sprintf("%s does not have transfer permissions for %s", owner.String(), rDenomNoAttr),
 		},
 		{
 			name: "restricted marker with required attributes but none match",
@@ -284,7 +284,7 @@ func TestSendRestrictionFn(t *testing.T) {
 			from:   addrWithDeposit,
 			to:     rMarkerNoAttr.GetAddress(),
 			amt:    cz(c(1, rDenomNoAttr)),
-			expErr: addrWithDeposit.String() + " does not have transfer permissions",
+			expErr: addrWithDeposit.String() + " does not have transfer access for " + rDenomNoAttr,
 		},
 		{
 			name: "send to another marker with transfer on denom but no deposit on to",
@@ -299,7 +299,7 @@ func TestSendRestrictionFn(t *testing.T) {
 			from:   addrWithDeposit,
 			to:     rMarker1Attr.GetAddress(),
 			amt:    cz(c(1, rDenomNoAttr)),
-			expErr: addrWithDeposit.String() + " does not have transfer permissions",
+			expErr: addrWithDeposit.String() + " does not have transfer access for " + rDenomNoAttr,
 		},
 		{
 			name:   "send to another marker with transfer on denom and deposit on to",
@@ -328,14 +328,14 @@ func TestSendRestrictionFn(t *testing.T) {
 			from:   addrWithBypass,
 			to:     rMarker1Attr.GetAddress(),
 			amt:    cz(c(1, rDenom1Attr)),
-			expErr: addrWithBypass.String() + " does not have transfer permissions",
+			expErr: addrWithBypass.String() + " does not have transfer access for " + rDenom1Attr,
 		},
 		{
 			name:   "to marker without req attrs from addr with bypass",
 			from:   addrWithBypass,
 			to:     rMarkerNoAttr.GetAddress(),
 			amt:    cz(c(1, rDenomNoAttr)),
-			expErr: addrWithBypass.String() + " does not have transfer permissions",
+			expErr: addrWithBypass.String() + " does not have transfer access for " + rDenomNoAttr,
 		},
 		{
 			name:   "no req attrs from addr with bypass",
@@ -357,7 +357,7 @@ func TestSendRestrictionFn(t *testing.T) {
 			from:   addrOther,
 			to:     addrWithBypass,
 			amt:    cz(c(1, rDenomNoAttr)),
-			expErr: addrOther.String() + " does not have transfer permissions",
+			expErr: addrOther.String() + " does not have transfer permissions for " + rDenomNoAttr,
 		},
 		{
 			name:   "no req attrs to addr with bypass from with transfer",
@@ -892,7 +892,7 @@ func TestQuarantineOfRestrictedCoins(t *testing.T) {
 	}
 	setAttr(t, addrQWithAttr)
 
-	noTransErr := addrWithoutTransfer.String() + " does not have transfer permissions"
+	noTransErr := addrWithoutTransfer.String() + " does not have transfer permissions for " + denomNoReqAttr
 	noAttrErr := func(addr sdk.AccAddress) string {
 		return fmt.Sprintf("address %s does not contain the %q required attribute: %q", addr, denom1ReqAttr, reqAttr)
 	}