Improve metadata scope nav cli command documentation (#2160)

* add expanded nav documentation for scope

* changelog

* add volume to metadata nav protos

* add volume parameter to metadata nav

* add another changelog for nav example

* Record net value asset only when explicitly set
by user when adding marker #2030

* fix metadata nav test argument format

* lint

* ensure metadata nav uses volume of 1
special handling of unset volume in nav to be one but allow zero for backwards compatibility

* fix nav event volume

* metadata scope set nav event documentation

---------

Co-authored-by: Carlton Hanna <nullpointer0x00@gmail.com>

.changelog/unreleased/client-breaking/2158-fix-metadata-nav-example.md

@@ -0,0 +1,4 @@
+* Fixes the metadata nav cli command example to use the correct module name [#2058](https://github.com/provenance-io/provenance/issues/2058)
+  During this fix it was discovered that the volume parameter was not present but was required for proper price ratios.  The volume
+  parameter has been added to the NAV entry and when not present a default value of 1 (which should be the most common case for a scope) is
+  used instead.

.changelog/unreleased/improvements/2030-explicit-navs.md

@@ -0,0 +1 @@
+* Only set a NAV record when explicitly provided. [#2030](https://github.com/provenance-io/provenance/issues/2030).

.changelog/unreleased/improvements/2134-metadata-nav-doc.md

@@ -0,0 +1 @@
+* Address missing documentation on metadata/scope nav command [#2134](https://github.com/provenance-io/provenance/issues/2134).

app/scope_navs_updater.go

@@ -72,7 +72,7 @@ func ReadScopeNAVs(fileName string) ([]ScopeNAV, error) {
 
 		asset := ScopeNAV{
 			ScopeUUID:     scopeUUID,
-			NetAssetValue: metadatatypes.NewNetAssetValue(price),
+			NetAssetValue: metadatatypes.NewNetAssetValue(price, 1),
 			Height:        height,
 		}
 

app/scope_navs_updater_test.go

@@ -23,12 +23,12 @@ func TestReadScopeNAVs(t *testing.T) {
 			expCount: 1101,
 			expFirst: ScopeNAV{
 				ScopeUUID:     "2f389a9f-873d-4920-85a6-7734f27e1738",
-				NetAssetValue: metadatatypes.NewNetAssetValue(sdk.NewInt64Coin(metadatatypes.UsdDenom, 398820670)),
+				NetAssetValue: metadatatypes.NewNetAssetValue(sdk.NewInt64Coin(metadatatypes.UsdDenom, 398820670), 1),
 				Height:        23056719,
 			},
 			expLast: ScopeNAV{
 				ScopeUUID:     "65939db0-6d7a-42ef-9443-378304d33225",
-				NetAssetValue: metadatatypes.NewNetAssetValue(sdk.NewInt64Coin(metadatatypes.UsdDenom, 93661920)),
+				NetAssetValue: metadatatypes.NewNetAssetValue(sdk.NewInt64Coin(metadatatypes.UsdDenom, 93661920), 1),
 				Height:        23056719,
 			},
 		},
@@ -37,12 +37,12 @@ func TestReadScopeNAVs(t *testing.T) {
 			expCount: 215558,
 			expFirst: ScopeNAV{
 				ScopeUUID:     "b0b97639-5ecf-4808-b679-99c11a5cda47",
-				NetAssetValue: metadatatypes.NewNetAssetValue(sdk.NewInt64Coin(metadatatypes.UsdDenom, 47395000)),
+				NetAssetValue: metadatatypes.NewNetAssetValue(sdk.NewInt64Coin(metadatatypes.UsdDenom, 47395000), 1),
 				Height:        14871216,
 			},
 			expLast: ScopeNAV{
 				ScopeUUID:     "98503480-12be-4142-bd9d-e80c6e017e22",
-				NetAssetValue: metadatatypes.NewNetAssetValue(sdk.NewInt64Coin(metadatatypes.UsdDenom, 43768160)),
+				NetAssetValue: metadatatypes.NewNetAssetValue(sdk.NewInt64Coin(metadatatypes.UsdDenom, 43768160), 1),
 				Height:        9787583,
 			},
 		},

proto/provenance/metadata/v1/events.proto

@@ -181,4 +181,5 @@ message EventSetNetAssetValue {
   string scope_id = 1;
   string price    = 2;
   string source   = 3;
+  string volume   = 4;
 }

proto/provenance/metadata/v1/scope.proto

@@ -229,4 +229,8 @@ message NetAssetValue {
   cosmos.base.v1beta1.Coin price = 1 [(gogoproto.nullable) = false];
   // updated_block_height is the block height of last update
   uint64 updated_block_height = 2;
+  // volume is the number of scope instances that were purchased for the price
+  // Typically this will be null (equivalent to one) or one.  The only reason this would be more than
+  // one is for cases where the precision of the price denom is insufficient to represent the actual price
+  uint64 volume = 3;
 }

x/marker/keeper/marker.go

@@ -447,9 +447,6 @@ func (k Keeper) FinalizeMarker(ctx sdk.Context, caller sdk.Address, denom string
 	if err != nil {
 		return err
 	}
-	if len(navs) == 0 {
-		return fmt.Errorf("marker %v does not have any net asset values assigned", denom)
-	}
 
 	// transition to finalized state ... then to active once mint is complete
 	if err = m.SetStatus(types.StatusFinalized); err != nil {

x/marker/keeper/msg_server.go

@@ -114,12 +114,17 @@ func (k msgServer) AddMarker(goCtx context.Context, msg *types.MsgAddMarkerReque
 		return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error())
 	}
 
-	usdMills := sdkmath.NewIntFromUint64(msg.UsdMills)
-	nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills), msg.Volume)
-	err = k.AddSetNetAssetValues(ctx, ma, []types.NetAssetValue{nav}, types.ModuleName)
-	if err != nil {
-		return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error())
+	// Only create a NAV entry if an explicit value is given for a NAV.  If a zero value is desired this can be set explicitly in a followup call.
+	// This check prevents a proliferation of incorrect NAV entries being recorded when setting up markers.
+	if msg.UsdMills > 0 {
+		usdMills := sdkmath.NewIntFromUint64(msg.UsdMills)
+		nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills), msg.Volume)
+		err = k.AddSetNetAssetValues(ctx, ma, []types.NetAssetValue{nav}, types.ModuleName)
+		if err != nil {
+			return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error())
+		}
 	}
+
 	// Note: The status can only be Active if this is being done via gov prop.
 	if ma.Status == types.StatusActive {
 		// Active markers should have supply set.
@@ -517,10 +522,14 @@ func (k msgServer) AddFinalizeActivateMarker(goCtx context.Context, msg *types.M
 		normalizedReqAttrs,
 	)
 
-	usdMills := sdkmath.NewIntFromUint64(msg.UsdMills)
-	err = k.AddSetNetAssetValues(ctx, ma, []types.NetAssetValue{types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills), msg.Volume)}, types.ModuleName)
-	if err != nil {
-		return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error())
+	// Only create a NAV entry if an explicit value is given for a NAV.  If a zero value is desired this can be set explicitly in a followup call.
+	// This check prevents a proliferation of incorrect NAV entries being recorded when setting up markers.
+	if msg.UsdMills > 0 {
+		usdMills := sdkmath.NewIntFromUint64(msg.UsdMills)
+		err = k.AddSetNetAssetValues(ctx, ma, []types.NetAssetValue{types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills), msg.Volume)}, types.ModuleName)
+		if err != nil {
+			return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error())
+		}
 	}
 
 	if err := k.Keeper.AddFinalizeAndActivateMarker(ctx, ma); err != nil {

x/marker/keeper/msg_server_test.go

@@ -108,12 +108,6 @@ func (s *MsgServerTestSuite) TestMsgAddMarkerRequest() {
 					Manager:    s.owner1,
 					MarkerType: types.MarkerType_Coin.String(),
 				},
-				&types.EventSetNetAssetValue{
-					Denom:  denom,
-					Price:  "0usd",
-					Volume: "0",
-					Source: types.ModuleName,
-				},
 			},
 		},
 		{
@@ -212,12 +206,6 @@ func (s *MsgServerTestSuite) TestMsgAddMarkerRequest() {
 					Manager:    s.owner1,
 					MarkerType: types.MarkerType_Coin.String(),
 				},
-				&types.EventSetNetAssetValue{
-					Denom:  denomWithDashPeriod,
-					Price:  "0usd",
-					Volume: "0",
-					Source: types.ModuleName,
-				},
 			},
 		},
 		{
@@ -242,12 +230,6 @@ func (s *MsgServerTestSuite) TestMsgAddMarkerRequest() {
 					Manager:    s.owner1,
 					MarkerType: types.MarkerType_RestrictedCoin.String(),
 				},
-				&types.EventSetNetAssetValue{
-					Denom:  rdenom,
-					Price:  "0usd",
-					Volume: "0",
-					Source: types.ModuleName,
-				},
 			},
 		},
 	}
@@ -311,11 +293,6 @@ func (s *MsgServerTestSuite) TestMsgFinalizeMarkerRequest() {
 		msg    types.MsgFinalizeRequest
 		expErr string
 	}{
-		{
-			name:   "marker does not have net asset value",
-			msg:    types.MsgFinalizeRequest{Denom: noNavMarker.Denom, Administrator: authUser.String()},
-			expErr: "marker nonav does not have any net asset values assigned: invalid request",
-		},
 		{
 			name: "successfully finalize",
 			msg:  types.MsgFinalizeRequest{Denom: validMarker.Denom, Administrator: authUser.String()},

x/metadata/client/cli/cli_test.go

@@ -3860,7 +3860,7 @@ func (s *IntegrationCLITestSuite) TestGetCmdAddNetAssetValues() {
 		},
 		{
 			name: "successful with multi net asset values",
-			args: argsWStdFlags(scopeID, "1usd,2jackthecat"),
+			args: argsWStdFlags(scopeID, "1usd;2jackthecat"),
 		},
 	}
 
@@ -3924,17 +3924,29 @@ func (s *IntegrationCLITestSuite) TestParseNetAssertValueString() {
 			expErr:         "invalid net asset value coin : notacoin",
 			expResult:      []types.NetAssetValue{},
 		},
+		{
+			name:           "invalid volume (previous format of comma separated coins is invalid)",
+			netAssetValues: "1hotdog,5cheesedog",
+			expErr:         "invalid volume : 5cheesedog",
+			expResult:      []types.NetAssetValue{},
+		},
 		{
 			name:           "successfully parse single nav",
+			netAssetValues: "1hotdog,10",
+			expErr:         "",
+			expResult:      []types.NetAssetValue{{Price: sdk.NewInt64Coin("hotdog", 1), Volume: 10}},
+		},
+		{
+			name:           "successfully parse single nav (no volume)",
 			netAssetValues: "1hotdog",
 			expErr:         "",
-			expResult:      []types.NetAssetValue{{Price: sdk.NewInt64Coin("hotdog", 1)}},
+			expResult:      []types.NetAssetValue{{Price: sdk.NewInt64Coin("hotdog", 1), Volume: 1}},
 		},
 		{
 			name:           "successfully parse multi nav",
-			netAssetValues: "1hotdog,20jackthecat",
+			netAssetValues: "1hotdog,20;20jackthecat",
 			expErr:         "",
-			expResult:      []types.NetAssetValue{{Price: sdk.NewInt64Coin("hotdog", 1)}, {Price: sdk.NewInt64Coin("jackthecat", 20)}},
+			expResult:      []types.NetAssetValue{{Price: sdk.NewInt64Coin("hotdog", 1), Volume: 20}, {Price: sdk.NewInt64Coin("jackthecat", 20), Volume: 1}},
 		},
 	}
 	for _, tc := range testCases {

x/metadata/client/cli/tx.go

@@ -2,7 +2,9 @@ package cli
 
 import (
 	"encoding/base64"
+	"errors"
 	"fmt"
+	"strconv"
 	"strings"
 
 	"github.com/google/uuid"
@@ -1190,11 +1192,47 @@ $ %[1]s tx metadata account-data %[2]s --%[5]s
 // GetCmdAddNetAssetValues returns a CLI command for adding/updating scopes net asset values.
 func GetCmdAddNetAssetValues() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:     "add-net-asset-values <scope-metadata-address> " + attrcli.AccountDataFlagsUse,
-		Aliases: []string{"add-navs", "anavs"},
-		Short:   "Add/updates net asset values for a scope",
-		Example: fmt.Sprintf(`$ %[1]s tx marker add-net-asset-values scope1234... 1usd,1;2nhash,3`,
-			version.AppName),
+		Use:     "add-net-asset-values <scope-metadata-address> <valuation[;valuation...]>",
+		Aliases: []string{"add-nav", "nav"},
+		Short:   "Provide net asset value for a scope",
+		Long: `
+Provide net asset valuation for a scope. Net asset values are used to establish 
+the relative value of the digital asset in relation to other assets or currencies.
+
+Net asset values are expressed as a ratio between an amount of coin paid (price) 
+and the scope (generally one) which are considered equivalent in value.
+
+The denomination of the amount paid (price) must either:
+
+  1) Exist on-chain as a marker, or
+  2) Be supplied as [1000usd], an integer valued in mils (1000 mils = $1 USD).
+
+IMPORTANT: The net asset value contains a volume which should be set to one
+in almost all cases. All values must be represented as whole integers. If a 
+decimal value is required, adjust the ratio between the price and volume as the
+least common denominator to achieve the desired precision.
+`,
+		Example: fmt.Sprintf(`
+  Set a value of $1 (Note USD is denominated in mils)
+  $ %[1]s tx %[2]s add-net-asset-values %[3]s 1000usd
+
+  Set a value of $1 (Note USD is denominated in mils) and include optional volume
+  $ %[1]s tx %[2]s add-net-asset-values %[3]s 1000usd,1
+
+  Provide more than one valuation in a single call
+  $ %[1]s tx %[2]s add-net-asset-values %[3]s 1000usd;5000000000nhash,1
+
+  Valuation for asset with volumes greater than 1 to adjust for high value price denom
+  $ %[1]s tx %[2]s add-net-asset-values %[3]s 1btc,60000
+
+  Note: When the valuations are recorded, each will indicate the address of the admin
+  who provided the value. This will be published in the associated event data and 
+  captured in the NAV record.  For NAVs set by other modules such as x/exchange the
+  protocol will indicate these sources.  This separates established values from
+  the owner (self-attestation) from those set through blockchain transactions.
+		`,
+			version.AppName, types.ModuleName, "scope1qzhp...tsk0cn"),
+
 		Args: cobra.ExactArgs(2),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			clientCtx, err := client.GetClientTxContext(cmd)
@@ -1264,19 +1302,31 @@ func validateAccAddress(addr, argName string) (string, error) {
 	return addr, nil
 }
 
-// ParseNetAssetValueString splits string (example 1hotdog,2jackthecat) to list of NetAssetValue's
+// ParseNetAssetValueString splits string (example 1hotdog,1;2jackthecat100,...) to list of NetAssetValue's
 func ParseNetAssetValueString(netAssetValuesString string) ([]types.NetAssetValue, error) {
-	navs := strings.Split(netAssetValuesString, ",")
+	navs := strings.Split(netAssetValuesString, ";")
 	if len(navs) == 1 && len(navs[0]) == 0 {
 		return []types.NetAssetValue{}, nil
 	}
 	netAssetValues := make([]types.NetAssetValue, len(navs))
 	for i, nav := range navs {
-		coin, err := sdk.ParseCoinNormalized(nav)
+		parts := strings.Split(nav, ",")
+		if len(parts) != 1 && len(parts) != 2 {
+			return []types.NetAssetValue{}, errors.New("invalid net asset value, expected [coin,volume] or [coin]")
+		}
+		coin, err := sdk.ParseCoinNormalized(parts[0])
 		if err != nil {
-			return []types.NetAssetValue{}, fmt.Errorf("invalid net asset value coin : %s", nav)
+			return []types.NetAssetValue{}, fmt.Errorf("invalid net asset value coin : %s", parts[0])
+		}
+		if len(parts) == 2 {
+			volume, err := strconv.ParseUint(parts[1], 10, 64)
+			if err != nil || volume < 1 {
+				return []types.NetAssetValue{}, fmt.Errorf("invalid volume : %s", parts[1])
+			}
+			netAssetValues[i] = types.NewNetAssetValue(coin, volume)
+		} else {
+			netAssetValues[i] = types.NewNetAssetValue(coin, 1)
 		}
-		netAssetValues[i] = types.NewNetAssetValue(coin)
 	}
 	return netAssetValues, nil
 }

x/metadata/keeper/genesis.go

@@ -67,7 +67,12 @@ func (k Keeper) InitGenesis(ctx sdk.Context, data *types.GenesisState) {
 			if err != nil {
 				panic(err)
 			}
-			err = k.SetNetAssetValue(ctx, address, types.NewNetAssetValue(nav.Price), types.ModuleName)
+			// Extra guard here in case volume is null or invalid
+			volume := nav.GetVolume()
+			if volume < 1 {
+				volume = 1
+			}
+			err = k.SetNetAssetValue(ctx, address, types.NewNetAssetValue(nav.Price, volume), types.ModuleName)
 			if err != nil {
 				panic(err)
 			}

x/metadata/keeper/msg_server.go

@@ -46,11 +46,16 @@ func (k msgServer) WriteScope(
 		return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error())
 	}
 
-	usdMills := sdkmath.NewIntFromUint64(msg.UsdMills)
-	nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills))
-	err := k.AddSetNetAssetValues(ctx, msg.Scope.ScopeId, []types.NetAssetValue{nav}, types.ModuleName)
-	if err != nil {
-		return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error())
+	// Do not set a NAV entry at this time unless a value greater than zero is specified.  This avoids the common case of
+	// not having a NAV entry value at hand during a scope write request.  A zero value can still be set explicitly with
+	// an add NAV call made separately.
+	if msg.UsdMills > 0 {
+		usdMills := sdkmath.NewIntFromUint64(msg.UsdMills)
+		nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills), 1)
+		err := k.AddSetNetAssetValues(ctx, msg.Scope.ScopeId, []types.NetAssetValue{nav}, types.ModuleName)
+		if err != nil {
+			return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error())
+		}
 	}
 
 	k.SetScope(ctx, msg.Scope)

x/metadata/keeper/scope.go

@@ -701,7 +701,12 @@ func (k Keeper) SetNetAssetValue(ctx sdk.Context, scopeID types.MetadataAddress,
 		return err
 	}
 
-	setNetAssetValueEvent := types.NewEventSetNetAssetValue(scopeID, netAssetValue.Price, source)
+	// Since this field was added we need to ensure the default value matches the previous behavior of always presuming one is used.
+	if netAssetValue.Volume < 1 {
+		netAssetValue.Volume = 1
+	}
+
+	setNetAssetValueEvent := types.NewEventSetNetAssetValue(scopeID, netAssetValue.Price, netAssetValue.Volume, source)
 	if err := ctx.EventManager().EmitTypedEvent(setNetAssetValueEvent); err != nil {
 		return err
 	}
@@ -757,7 +762,7 @@ func (k Keeper) SetNetAssetValueWithBlockHeight(ctx sdk.Context, scopeID types.M
 		return err
 	}
 
-	setNetAssetValueEvent := types.NewEventSetNetAssetValue(scopeID, netAssetValue.Price, source)
+	setNetAssetValueEvent := types.NewEventSetNetAssetValue(scopeID, netAssetValue.Price, netAssetValue.Volume, source)
 	if err := ctx.EventManager().EmitTypedEvent(setNetAssetValueEvent); err != nil {
 		return err
 	}

x/metadata/keeper/scope_test.go

@@ -2550,7 +2550,7 @@ func (s *ScopeKeeperTestSuite) TestSetNetAssetValue() {
 			var expErrs []string
 			var expEvents sdk.Events
 			if len(tc.expErr) == 0 {
-				event := types.NewEventSetNetAssetValue(scopeID, tc.netAssetValue.Price, "test")
+				event := types.NewEventSetNetAssetValue(scopeID, tc.netAssetValue.Price, 1, "test")
 				eventU, err := sdk.TypedEventToEvent(event)
 				s.Require().NoError(err, "TypedEventToEvent(NewEventSetNetAssetValue)")
 				expEvents = sdk.Events{eventU}
@@ -2588,6 +2588,7 @@ func (s *ScopeKeeperTestSuite) TestRemoveNetAssetValues() {
 					Denom:  "usd",
 					Amount: sdkmath.NewInt(1000),
 				},
+				Volume: 1,
 			},
 			expErr: "",
 		},
@@ -2605,7 +2606,7 @@ func (s *ScopeKeeperTestSuite) TestRemoveNetAssetValues() {
 			})
 			s.Require().NoError(err, "IterateNetAssetValues err")
 			s.Require().Len(netAssetValues, 1, "Should have added a NAV")
-			s.Require().Equal(tc.netAssetValue, netAssetValues[0], "Should have added the test case nave.")
+			s.Require().Equal(tc.netAssetValue, netAssetValues[0], "Should have added the test case nav.")
 			s.app.MetadataKeeper.RemoveNetAssetValues(ctx, tc.scopeID)
 			netAssetValues = []types.NetAssetValue{}
 			err = s.app.MetadataKeeper.IterateNetAssetValues(ctx, tc.scopeID, func(state types.NetAssetValue) (stop bool) {