Filter and Sorting ignores scale in reward object

[jordaniza]

Describe the bug

Reward is (partially) defined as:

reward: {
  amount: number,
  scale: number
}

And on the bountyboard/frontend, we calculate bounty $BANK value as reward.amount / (10 ^ reward.scale)
So an example, the bounty bank value would be:

5000 amount, 0 scale = 5000 BANK
10500 amount, 3 scale = 10.5 BANK

As it currently stands, this is not accounted for in the filters or the sort criteria. The bounty board just pulls in 5k vs 10.5k, which is incorrect.

To Reproduce
Steps to reproduce the behavior:

1. Add 2 bounties, 1 for 5000 with a scale of 0, 1 for 10,000 with a scale of 1 (reward of 1000)
2. Start sorting or filtering select "greater than 2000"
3. You will still see the 1000 in the frontend

Expected behavior
Sorting and filtering should adjust for decimal places provided by the scale field

There's a couple of ways we could do this:

1. Fancy Mongo Queries

You can write mongo queries to handle this in the case of "reward", HOWEVER, this kinda sucks:

• You need to do this for filtering ($lte, $gte), AND for sorting
• You need to treat "reward" entirely differently to other fields, so you add a lot of conditions

// this is JUST for the $gt case
db.bounties.find({
  $expr: {
    $gt: [
      {
        $divide: [
          "$reward.amount",
          {
            $pow: [10, "$reward.scale"]
          }
        ]
      },
    AMOUNT_OF_BANK_GOES_HERE_AS_NUMBER
  ]
}});

2. Client side
   We could just pull all the data and do a second stage filter on the client, this still involves writing a lot of extra logic

3. Add a new field to the bounties reward object

• I'd say this is the most sensible, here we just add a field like total which is simply calculated on Insert/Update as amount / 10 ^ scale

• It's then trivial to just filter by "reward.total", and sort by "reward.total"

[Behold3th]

I think the amount and scale fields as persistent fields in our database should be reconsidered. I see how scale can help the customer experience, but that is orthogonal to our database needs:

Our database is our source of truth, and we should strive to:

• eliminate ambiguity
• eliminate data duplication

amount and scale are not overlapping fields, but together they present the same information that the proposed total field represents.

I would propose changing the reward field to:

reward: {
  amount: number,
}

However, given our dependency on the DEGEN team and our release date, I think our priorities are to implement a stable release, and clean up minor technical debt later.

Thus, I think @jordaniza 's solution of adding the total field is the way to go for now, with the added responsibility of opening up an issue to think through the implications of simplifying our reward schema

[jordaniza]

This makes sense, we can add fairly cleanly to our own API without affecting DEGEN if this is some sort of stored procedure (I'm assuming Mongo has these).

Definitely a pre-release task but shouldn't be too long to test and deploy, just the challenge of retroactively running on the main production DB.

I like your longer term view. It doesn't make much sense to separate scale in my view.

@Behold3th, can you bring this up in the standup, it touches a couple of teams and skillsets so would be good to see who we need to get involved to do this cleanly.

[jordaniza]

Been digging into this a bit more after speaking to @Behold3th

The tl:dr:

• Looks to me like DEGEN doesn't perform any transformations, prior to saving in the DB (good news for us)
• However, this is not reflected in the test database, which is where I found the issue (medium news for us)
• Also, the confirmation message sent to the user DOES seem to be adjusted for scale (not great news for us)
• Also also, DEGEN looks like it will reject any currency that does not regexp match to BANK (bad news for us)

Couple of changes needed on the DEGEN side: flagging @nonsensecodes for visibility.

My suggestions:

• Confirm with the DEGEN team that our understanding of the below is correct (if so, no action needed on BB side, we can just use reward.amount for sorting)
• Check how we want to handle multi-currency support with new DAOs, we need to allow such currencies to pass the regxpr test in DEGEN
• Revise the DEGEN messaging for decimal point denominated rewards, to check confirmation messages are being sent to the user correctly
  Full details:

How a bounty is created in DEGEN

Pretty straightforward, DEGEN runs a couple of commands:

Create the params:

// src/app/commands/bounty/Bounty.ts (255)
				params = this.buildBountyCreateNewParams(ctx.options.create);
				console.log('/bounty create ' + params.title);
				command = CreateNewBounty(guildMember, params);

The params come from this function in the same file:

// (302)
	buildBountyCreateNewParams(ctxOptions: { [key: string]: any }): BountyCreateNew {
		const [reward, symbol] = (ctxOptions.reward != null) ? ctxOptions.reward.split(' ') : [null, null];
		const copies = (ctxOptions.copies == null || ctxOptions.copies <= 0) ? 1 : ctxOptions.copies;
		let scale = reward.split('.')[1]?.length;
		scale = (scale != null) ? scale : 0;
		return {
			title: ctxOptions.title,
			reward: {
				amount: reward,
				currencySymbol: symbol,
				scale: scale,
				amountWithoutScale:  reward.replace('.', ''),
			},
			copies: copies,
		};

Note: the reward here is saved as a string (for now) and is NOT adjusted by scale, instead the scale is logged as a param

CreateNewBounty is a default export from src/app/service/bounty/create/CreateNewBounty.ts, it takes the params object and uses it as so, to calculate reward:

// first it validates the reward:
	await BountyUtils.validateReward(guildMember, reward);

Pretty important point here, currently DEGEN will reject anything not denominated in BANK:

// src/app/utils/BountyUtils.ts (65)
	async validateReward(guildMember: GuildMember, reward: BountyReward): Promise<void> {
		const ALLOWED_CURRENCIES = ['BANK'];
		const allowedRegex = new RegExp(ALLOWED_CURRENCIES.join('|'), 'i');
		const MAXIMUM_REWARD = 100000000.00;

		if (isNaN(reward.amount) || reward.amount <= 0 || reward.amount > MAXIMUM_REWARD
			|| !allowedRegex.test(reward.currencySymbol)) {
			await guildMember.send({
				content: `<@${guildMember.user.id}>\n` +
					'Please enter a valid reward value: \n ' +
					'- 100 million maximum currency\n ' +
					'- accepted currencies: ETH, BANK',
			});
			throw new ValidationError('Please try another reward.');
		}
	},

Anyway, once the reward is validated, DEGEN pushes to the DB:

// src/app/service/bounty/create/CreateNewBounty.ts (71)
	const db: Db = await dbInstance.dbConnect(constants.DB_NAME_BOUNTY_BOARD);
	const dbBounty = db.collection(constants.DB_COLLECTION_BOUNTIES);

	const listOfPrepBounties = [];
	for (let i = 0; i < params.copies; i++) {
		listOfPrepBounties.push(generateBountyRecord(params, guildMember));
	}

	const dbInsertResult = await dbBounty.insertMany(listOfPrepBounties, { ordered: false });

//... (121)
export const generateBountyRecord = (bountyParams: BountyCreateNew, guildMember: GuildMember): any => {
    // ...
		reward: {
			currency: bountyParams.reward.currencySymbol,
			amount: new Int32(bountyParams.reward.amount),
			scale: new Int32(bountyParams.reward.scale),
		},

But it displays a different result to the user:

// (87)
	const messageOptions: MessageOptions = {
		// ...
				{ name: 'Reward', value: BountyUtils.formatBountyAmount(newBounty.reward.amount, newBounty.reward.scale) + ' ' + newBounty.reward.currency.toUpperCase(), inline: true },
            // ...
	};

	await guildMember.send('Thank you! Does this look right?');
	const message: Message = await guildMember.send(messageOptions);

Where formatBountyAmount is the scale adjustment I found earlier (I THINK this will be incorrect, but might be missing something)

// src/app/utils/BountyUtils.ts (167)
	formatBountyAmount(amount: number, scale: number): string {
		return (amount / 10 ** scale).toString();
	},

[jordaniza]

Closing as addressed in bountybot integration