Percentage based shareholders payout might not be the right approach

[fmorency]

Context

POA admin will use the web app to mint and send specific amounts of tokens to shareholders. The current implementation of the payout algorithm is based on percentages of the total amount, not individual amounts, which can lead to numerical imprecision.

Example

Let $sh1=1$ and $sh2=1,000,000$ where the unit of $sh1$ and $sh2$ is umfx

Let $t = sh1 + sh2 = 1,000,001$ be the total amount to be distributed.

Let $p1=\frac{sh1}{t}=\frac{1}{1,000,001}$ and $p2=\frac{sh2}{t}=\frac{1,000,000}{1,000,001}$ be the percentages of the total amount of $sh1$ and $sh2$ respectively.

The payout algorithm uses the int32 type to represent percentage values. The percentage field has 6 decimal places, meaning the value $m=100,000,000$ is used to represent $100\%$.

The percentage value passed to the payout algorithm can be computed as

$p1'=\frac{sh1*m}{t}=\frac{100,000,000}{1,000,001}=99.\overline{999900000099}$

$p2'=\frac{sh2*m}{t}=\frac{100,000,000,000,000}{1,000,001}=99999900.\overline{000099999900}$

Problems

• 6 decimal places are not enough to represent repeating numbers.
• Loss of precision when converting $p1'$ and $p2'$ to integers $p1_i'$ and $p2_i'$
• $p1_i'+p2_i'=99,999,999$ and NOT $100,000,000$
• The resulting amount being distributed is NOT what is expected

Edit: Using sdkmath and banker rounding works with the example above but I'm still not convinced we won't encounter other issues. More tests are needed.

Edit2: $sh1=1$ and $sh2=1,000,000,000,000$ breaks.

Edit3: The percentage-based approach is prone to rounding, truncation error, and precision issue. While the method might work sometimes, it is not robust enough to work all the time. We need the approach to work all the time, without any truncation error or loss in precision. The ideal approach would take

{
  "addr1": "amount1",
  "addr2": "amount2",
  ...
  "addrn": "amountn"
}

as input and mint those amounts directly without any other manipulation other than multiplication by a given decimal place, e.g., 1e6. The amount* MUST be validated before minting to ensure they are in range and valid.

Relates #27

[fmorency]

@Reecepbcups, any update?

[fmorency]

Plan

• Drop []*ShareHolders from the module params
• Drop CalculateShareHolderTokenPayout
• Modify MsgPayoutStakeholders so it takes a map of AddressString:Coins in addition to the authority
• Fix partial minting caused by errors. We should never perform partial minting.
• Uniformize language, i.e., shareholder vs stakeholder.

[Reecepbcups]

@fmorency That works too, but I was told in the original the percents approach was desired rather than having to specify the exact number of coins (so those could change dynamically) every signature.

The AddressString:Coins was the original approach I wanted to take but opted for this due to that.

It does put more on your (multisig) shoulders though, if that's reasonable I think it should be done. lmk if that's the desire and can change

[fmorency]

@Reecepbcups Interesting... I'll need to discuss that with your boss ;)

The AddressString:Coins approach is 10000% better. The benefits far outweighs the multisig argument.

Please proceed with the change if you have the bandwidth for it. I appreciate it!

[Reecepbcups]

@fmorency kk, and what about automatic inflation?

initial thoughts:

• Should there be a param of map<address, coin> which is paid per block?
• use the last payout and convert everyone to percents (then spread this out over a yearly inflation (when turned on)?

Either way automatic inflation requires some % based math, which now helps me recall why we did this design for both manual and automatic too. Else we are getting into 2 different designs for similar logic (i.e. more risk, overhead, etc)

[fmorency]

@Reecepbcups, at the moment, we don't have a use case for automatic inflation in a POA chain context. We can do a software upgrade if we ever need it or switch to POS.

[Reecepbcups]

oh okay if that is the case I will remove automatic inflation and just have manual payouts. Will get started on the refactor tonight

[fmorency]

@Reecepbcups I'm not sure if you deleted your comment but here's my take on

For payouts, would you like to have the ability to pay in other coins other than umfx or should that be a validation check (only umfx payouts)

We want the ability to mint any native chain coin, which would be umfx in our context.

[Reecepbcups]

yes I figured this was the case which is why I deleted :) leaving it open. I just finished final testing on the new approach, once passing Ill open for review