Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the bug in deposit #151

Merged
merged 7 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
perf: use unchecked to optimize calculate functions
test: rename asset with 24 decimals
  • Loading branch information
andreivladbrg committed Jun 3, 2024
commit 3cc9212d0ea28c0b4d2b7cd266919933f5d554b3
50 changes: 28 additions & 22 deletions src/libraries/Helpers.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import { Errors } from "./Errors.sol";
import { Broker } from "../types/DataTypes.sol";

/// @title Helpers
/// @notice Library with helper functions in {SablierFlo} contract.
/// @notice Library with helper functions in {SablierFlow} contract.
library Helpers {
using SafeCast for uint256;

/// @notice Calculates the normilized amount based on the asset's decimals.
/// @dev Changes the amount based on the asset's decimal difference from 18:
/// @notice Calculates the normalized amount based on the asset's decimals.
/// @dev Changes the transfer amount based on the asset's decimal difference from 18:
/// - if the asset has more decimals, the amount is reduced, also the transfer amount is rounded to zero after the
/// 18th decimal place
/// - if the asset has fewer decimals, the amount is increased
Expand All @@ -31,21 +31,25 @@ library Helpers {
return (transferAmount, transferAmount);
}

if (assetDecimals > 18) {
uint8 normalizingFactor = assetDecimals - 18;
uint128 factor = (10 ** normalizingFactor).toUint128();
unchecked {
if (assetDecimals > 18) {
uint8 normalizingFactor = assetDecimals - 18;
uint128 factor = (10 ** normalizingFactor).toUint128();

// If the number has 10.000..(asset decimals)..005, the transfer amount will be rounded to zero after the
// 18th decimal place, i.e. to 10.000..(asset decimals)..000. This is because we do not account for more
// than 18 decimals internally, which would otherwise lead to an excess of assets in our contract.
transferAmount = transferAmount / factor * factor;
// Normalize the amount to 18 decimals.
uint128 normalizedAmount = transferAmount / factor;

uint128 normalizedAmount = transferAmount / factor;
return (transferAmount, normalizedAmount);
} else {
uint8 normalizingFactor = 18 - assetDecimals;
uint128 normalizedAmount = (transferAmount * (10 ** normalizingFactor)).toUint128();
return (transferAmount, normalizedAmount);
// If the number has 10.000..(asset decimals)..005, the transfer amount will be rounded to zero after
// the 18th decimal place, i.e. to 10.000..(asset decimals)..000. This is because we do not account for
// more than 18 decimals internally, which would otherwise lead to an excess of assets in our contract.
transferAmount = normalizedAmount * factor;

return (transferAmount, normalizedAmount);
} else {
uint128 normalizingFactor = 18 - assetDecimals;
uint128 normalizedAmount = transferAmount * (10 ** normalizingFactor).toUint128();
return (transferAmount, normalizedAmount);
}
}
}

Expand All @@ -59,12 +63,14 @@ library Helpers {
return amount;
}

if (assetDecimals > 18) {
uint8 normalizingFactor = assetDecimals - 18;
return (amount * (10 ** normalizingFactor)).toUint128();
} else {
uint8 normalizingFactor = 18 - assetDecimals;
return (amount / (10 ** normalizingFactor)).toUint128();
unchecked {
if (assetDecimals > 18) {
uint8 normalizingFactor = assetDecimals - 18;
return (amount * (10 ** normalizingFactor)).toUint128();
} else {
uint8 normalizingFactor = 18 - assetDecimals;
return (amount / (10 ** normalizingFactor)).toUint128();
}
}
}

Expand Down
6 changes: 3 additions & 3 deletions test/Base.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ abstract contract Base_Test is Assertions, Constants, Events, Modifiers, Test, U
//////////////////////////////////////////////////////////////////////////*/

ERC20Mock internal assetWithoutDecimals = new ERC20Mock("Asset without decimals", "AWD", 0);
ERC20Mock internal assetWithMoreDecimals = new ERC20Mock("Asset without decimals", "AWD", 24);
ERC20Mock internal assetWith24Decimals = new ERC20Mock("Asset with more decimals", "AWMD", 24);
ERC20Mock internal dai = new ERC20Mock("Dai stablecoin", "DAI", 18);
ERC20Mock internal usdc = new ERC20Mock("USD Coin", "USDC", 6);
ERC20MissingReturn internal usdt = new ERC20MissingReturn("USDT stablecoin", "USDT", 6);
Expand Down Expand Up @@ -73,12 +73,12 @@ abstract contract Base_Test is Assertions, Constants, Events, Modifiers, Test, U
function createUser(string memory name) internal returns (address payable) {
address payable user = payable(makeAddr(name));
vm.deal({ account: user, newBalance: 100 ether });
deal({ token: address(assetWithMoreDecimals), to: user, give: 1_000_000e24 });
deal({ token: address(assetWith24Decimals), to: user, give: 1_000_000e24 });
deal({ token: address(dai), to: user, give: 1_000_000e18 });
deal({ token: address(usdc), to: user, give: 1_000_000e6 });
deal({ token: address(usdt), to: user, give: 1_000_000e18 });
resetPrank(user);
assetWithMoreDecimals.approve({ spender: address(flow), value: type(uint256).max });
assetWith24Decimals.approve({ spender: address(flow), value: type(uint256).max });
dai.approve({ spender: address(flow), value: type(uint256).max });
usdc.approve({ spender: address(flow), value: type(uint256).max });
usdt.approve({ spender: address(flow), value: type(uint256).max });
Expand Down
8 changes: 4 additions & 4 deletions test/integration/concrete/deposit/deposit.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,16 @@ contract Deposit_Integration_Concrete_Test is Integration_Test {
}

function test_Deposit_AssetMoreThan18Decimals_OnlyZeros() external {
uint256 streamId = createDefaultStreamWithAsset(IERC20(address(assetWithMoreDecimals)));
uint256 streamId = createDefaultStreamWithAsset(IERC20(address(assetWith24Decimals)));
uint128 transferAmount = 50_000e24;
_test_Deposit(streamId, assetWithMoreDecimals, transferAmount, 24);
_test_Deposit(streamId, assetWith24Decimals, transferAmount, 24);
}

function test_Deposit_AssetMoreThan18Decimals_NotOnlyZeros() external {
uint256 streamId = createDefaultStreamWithAsset(IERC20(address(assetWithMoreDecimals)));
uint256 streamId = createDefaultStreamWithAsset(IERC20(address(assetWith24Decimals)));
uint128 transferAmount = 50_000e24;
transferAmount += 5;
_test_Deposit(streamId, assetWithMoreDecimals, transferAmount, 24);
_test_Deposit(streamId, assetWith24Decimals, transferAmount, 24);
}

function _test_Deposit(uint256 streamId, IERC20 asset, uint128 transferAmount, uint8 assetDecimals) internal {
Expand Down