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.

Sign up for GitHub

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

Jump to bottom

Use safecast for any downcasting #2306

Merged
merged 4 commits into from
Nov 13, 2024
Merged

Use safecast for any downcasting #2306

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
466ee53
use safecasts for any downcasting
sparrowDom Nov 11, 2024
da80991
use safecast when downcasting from int256. Also fix tests
sparrowDom Nov 12, 2024
5586ea6
prettier
sparrowDom Nov 13, 2024
21f7eb7
clean up implementation
sparrowDom Nov 13, 2024
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
43 changes: 22 additions & 21 deletions contracts/contracts/token/OUSD.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { SafeCast } from "@openzeppelin/contracts/utils/math/SafeCast.sol";

contract OUSD is Governable {
using SafeCast for int256;
using SafeCast for uint256;

event TotalSupplyUpdatedHighres(
uint256 totalSupply,
Expand Down Expand Up @@ -260,11 +261,11 @@ contract OUSD is Governable {
(
int256 fromRebasingCreditsDiff,
int256 fromNonRebasingSupplyDiff
) = _adjustAccount(_from, -int256(_value));
) = _adjustAccount(_from, -_value.toInt256());
(
int256 toRebasingCreditsDiff,
int256 toNonRebasingSupplyDiff
) = _adjustAccount(_to, int256(_value));
) = _adjustAccount(_to, _value.toInt256());

_adjustGlobals(
fromRebasingCreditsDiff + toRebasingCreditsDiff,
Expand All @@ -277,41 +278,41 @@ contract OUSD is Governable {
returns (int256 rebasingCreditsDiff, int256 nonRebasingSupplyDiff)
{
RebaseOptions state = rebaseState[account];
int256 currentBalance = int256(balanceOf(account));
uint256 newBalance = (currentBalance + balanceChange).toUint256();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change away from uint256? A balance is a number that can only be positive. It also looks like this changes makes us have to do many more conversions later in the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of the check on line 283. Though you are right this could be done in a cleaner way. Done here: 21f7eb7

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Good catch.

int256 currentBalance = balanceOf(account).toInt256();
int256 newBalance = currentBalance + balanceChange;
if (newBalance < 0) {
revert("Transfer amount exceeds balance");
}
if (state == RebaseOptions.YieldDelegationSource) {
address target = yieldTo[account];
uint256 targetOldBalance = balanceOf(target);
uint256 targetNewCredits = _balanceToRebasingCredits(
targetOldBalance + newBalance
targetOldBalance + newBalance.toUint256()
);
rebasingCreditsDiff =
int256(targetNewCredits) -
int256(_creditBalances[target]);
targetNewCredits.toInt256() -
_creditBalances[target].toInt256();

_creditBalances[account] = newBalance;
_creditBalances[account] = newBalance.toUint256();
_creditBalances[target] = targetNewCredits;
alternativeCreditsPerToken[account] = 1e18;
} else if (state == RebaseOptions.YieldDelegationTarget) {
uint256 newCredits = _balanceToRebasingCredits(
newBalance + _creditBalances[yieldFrom[account]]
newBalance.toUint256() + _creditBalances[yieldFrom[account]]
);
rebasingCreditsDiff =
int256(newCredits) -
int256(_creditBalances[account]);
newCredits.toInt256() -
_creditBalances[account].toInt256();
_creditBalances[account] = newCredits;
} else if (_isNonRebasingAccount(account)) {
nonRebasingSupplyDiff = balanceChange;
alternativeCreditsPerToken[account] = 1e18;
_creditBalances[account] = newBalance;
_creditBalances[account] = newBalance.toUint256();
} else {
uint256 newCredits = _balanceToRebasingCredits(newBalance);
uint256 newCredits = _balanceToRebasingCredits(newBalance.toUint256());
rebasingCreditsDiff =
int256(newCredits) -
int256(_creditBalances[account]);
newCredits.toInt256() -
_creditBalances[account].toInt256();
_creditBalances[account] = newCredits;
}
}
Expand All @@ -321,16 +322,16 @@ contract OUSD is Governable {
int256 nonRebasingSupplyDiff
) internal {
if (rebasingCreditsDiff != 0) {
if (int256(_rebasingCredits) + rebasingCreditsDiff < 0) {
if (_rebasingCredits.toInt256() + rebasingCreditsDiff < 0) {
revert("rebasingCredits underflow");
}
_rebasingCredits = (int256(_rebasingCredits) + rebasingCreditsDiff).toUint256();
_rebasingCredits = (_rebasingCredits.toInt256() + rebasingCreditsDiff).toUint256();
}
if (nonRebasingSupplyDiff != 0) {
if (int256(nonRebasingSupply) + nonRebasingSupplyDiff < 0) {
if (nonRebasingSupply.toInt256() + nonRebasingSupplyDiff < 0) {
revert("nonRebasingSupply underflow");
}
nonRebasingSupply = (int256(nonRebasingSupply) + nonRebasingSupplyDiff).toUint256();
nonRebasingSupply = (nonRebasingSupply.toInt256() + nonRebasingSupplyDiff).toUint256();
}
}

Expand Down Expand Up @@ -427,7 +428,7 @@ contract OUSD is Governable {
(
int256 toRebasingCreditsDiff,
int256 toNonRebasingSupplyDiff
) = _adjustAccount(_account, int256(_amount));
) = _adjustAccount(_account, _amount.toInt256());
// Globals
_adjustGlobals(toRebasingCreditsDiff, toNonRebasingSupplyDiff);
_totalSupply = _totalSupply + _amount;
Expand Down Expand Up @@ -464,7 +465,7 @@ contract OUSD is Governable {
(
int256 toRebasingCreditsDiff,
int256 toNonRebasingSupplyDiff
) = _adjustAccount(_account, -int256(_amount));
) = _adjustAccount(_account, -_amount.toInt256());
// Globals
_adjustGlobals(toRebasingCreditsDiff, toNonRebasingSupplyDiff);
_totalSupply = _totalSupply - _amount;
Expand Down
2 changes: 1 addition & 1 deletion contracts/test/flipper/flipper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ describe("Flipper", function () {
// eslint-disable-next-line
`buyOusdWith${titleName}`
](ousdUnits("1"));
await expect(call).to.be.revertedWith("Transfer greater than balance");
await expect(call).to.be.revertedWith("Transfer amount exceeds balance");
}
);
});
Expand Down
2 changes: 1 addition & 1 deletion contracts/test/vault/oeth-vault.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1428,7 +1428,7 @@ describe("OETH Vault", function () {
.connect(josh)
.requestWithdrawal(dataBefore.userOeth.add(1));

await expect(tx).to.revertedWith("Remove exceeds balance");
await expect(tx).to.revertedWith("Transfer amount exceeds balance");
});
it("capital is paused", async () => {
const { oethVault, governor, josh } = fixture;
Expand Down
2 changes: 1 addition & 1 deletion contracts/test/vault/redeem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ describe("Vault Redeem", function () {
// Try to withdraw more than balance
await expect(
vault.connect(anna).redeem(ousdUnits("100.0"), 0)
).to.be.revertedWith("Remove exceeds balance");
).to.be.revertedWith("Transfer amount exceeds balance");
});

it("Should only allow Governor to set a redeem fee", async () => {
Expand Down
Loading