fix: rounding in the protocol's favor pt. 3

[0xJabberwock]

Description

After thoroughly reviewing the WeightedMath library in search of rounding inconsistencies, I've found some cases in which multiplications (mulUp / mulDown) or divisions (divUp / divDown) weren't favorable to the protocol.

For reference, BasePoolMath has undergone such review in #468, and StableMath in #487.

Type of change

• Bug fix
• New feature
• Breaking change
• Dependency changes
• Code refactor / cleanup
• Documentation or wording changes
• Other

Checklist:

• The diff is legible and has no extraneous changes
• Complex code has been commented, including external interfaces
• Tests have 100% code coverage
• The base branch is either main, or there's a description of how to merge

Issue Resolution

Closes #488.

[joaobrunoah]

@0xJabberwock , can you create some tests with the cases you found, to make sure the math doesn't break for these cases again?

[EndymionJkb]

It seems right, but it's getting late and I'm getting bleary eyed :) Similar text comments to those on StableMath.

[EndymionJkb]

Suggested change

	// which will be used later upon calculating the `nonTaxableAmount`
	// which will be used later when calculating the `nonTaxableAmount` for each token.

Comments should always end with a period (here and elsewhere).

[EndymionJkb]

Is using multiple returns more gas efficient, or do you just think it's clearer?

[0xJabberwock]

What is more gas efficient is not to declare the memory variable bptOut, that is achieved by using multiple returns. Besides, it keeps consistency with the same logic found in StableMath.

[EndymionJkb]

Suggested change

	// which will be used later upon calculating the `nonTaxableAmount`
	// which will be used later when calculating the `nonTaxableAmount` for each token.

[0xJabberwock]

In this case, the nonTaxableAmount is calculated later only for the token in.

[EndymionJkb]

Suggested change

	// `balanceRatio`; we prioritize incrementing the `nonTaxableAmount` over the `taxableAmount`.
	// `balanceRatio`; we prioritize maximizing the `nonTaxableAmount`.

As in StableMath, would be good to explain the reasoning here.

[0xJabberwock]

See #487 (comment).

[joaobrunoah]

I agree with Jeff about a fuzz testing (PR #488), or some sort of test that would fail for the code we changed, to ensure maintainability of the code.

Still, maybe it's out of scope for this PR (I opened #505 to deal with this).

[EndymionJkb]

I agree with Jeff about a fuzz testing (PR #488), or some sort of test that would fail for the code we changed, to ensure maintainability of the code.

Still, maybe it's out of scope for this PR (I opened #505 to deal with this).

It's probably not easy to make these tests, but I hesitate to change anything in our core math libraries without some assurance via testing that they're behaving as we expect. (In the ambiguous cases, we're betting that other "swiss cheese layers," such as adding/subtracting buffer wei to final results, will protect us when the rounding direction isn't clear.)

The tests (BasePool, Weighted, and Stable) could be done in a follow-up PR if that's easier.

[0xJabberwock]

Thanks for the feedback @joaobrunoah and @EndymionJkb!

I do agree that properly testing the rounding direction would be beneficial for the health of the codebase. Therefore, I'll be tackling #505 in an upcoming PR.

[EndymionJkb]

I do agree that properly testing the rounding direction would be beneficial for the health of the codebase. Therefore, I'll be tackling #505 in an upcoming PR.

Sounds good. You can resolve the comment things above so it's easier to see when it's complete, but as with the others, these should be fine once updated and CI passes, pending final validation with tests, per #505.