[PoC] Downcasting from u256 to u128

[josojo]

Currently this downcasting could corrupt the numbers. We need to make sure that this is not happening.

@bh2smith is currently writing a library for it

[fleupold]

Which downcasting are you talking about? Can you provide a link or more context?

[bh2smith]

Which downcasting are you talking about?

I believe this refers explicitly to the require statement at the bottom of the following function

dex-contracts/contracts/StablecoinConverter.sol

Lines 260 to 277 in 4667388

	function getExecutedSellAmount(
	uint128 executedBuyAmount,
	uint128 buyTokenPrice,
	uint128 sellTokenPrice
	) internal view returns (uint128) {
	// executedSellAmount = sellAmount * (1 - (1/feeDenominator))
	// = sellAmount - sellAmount/feeDenominator
	// = (sellAmount * feeDenominator) / feeDenominator - sellAmount/feeDenominator
	// = (sellAmount * feeDenominator - sellAmount) / feeDenominator
	// = (sellAmount * (feeDenominator - 1)) /feeDenominator
	// = (executedBuyAmount * buyTokenPrice / sellTokenPrice) * (feeDenominator - 1) / feeDenominator
	// in order to minimize rounding errors, the order is switched
	// = (executedBuyAmount * buyTokenPrice / feeDenominator) * (feeDenominator - 1) / sellTokenPrice
	uint256 sellAmount = (uint256(executedBuyAmount).mul(buyTokenPrice) / (feeDenominator - 1))
	.mul(feeDenominator) / sellTokenPrice;
	require(sellAmount < MAX_UINT128, "sellAmount too large");
	return uint128(sellAmount);
	}

I have opened a PR in OpenZeppelin repo that circumvents having to do this in a much simpler way. Take a look here!

OpenZeppelin/openzeppelin-contracts#1926

[bh2smith]

The decision for this sprint was to determine what the best approach is here while waiting for the next release (since OpenZeppelin.SafeCast wont appear till v2.5).

[fleupold]

I think we suggested to use OpenZeppelin as a npm git dependency (https://docs.npmjs.com/files/package.json#git-urls-as-dependencies) pinned to a tag/commit that includes the new SafeCast library.