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

[Part 2 of 2] feat: minor tweaks (add unchecked) #31

Merged
merged 11 commits into from
Jun 5, 2024
Merged

[Part 2 of 2] feat: minor tweaks (add unchecked) #31

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
33565dc
chore: minor gas opt safeCast
ChefMist Apr 29, 2024
b0cd33f
test: update test
ChefMist Apr 29, 2024
e1710a0
tweak: use unchecked
ChefMist Apr 29, 2024
87694f1
Merge branch 'main' into feat/tweaks
ChefMist May 7, 2024
bdfd2df
gas: updated forge gas
ChefMist May 7, 2024
c32004e
Merge branch 'feat/tweaks' into feat/tweaks-v2
ChefMist May 7, 2024
b1a02ef
test: updated forge gas
ChefMist May 7, 2024
c2e92b8
Merge branch 'main' into feat/tweaks-v2
ChefMist May 8, 2024
4bff164
gas: updated forge test --isolate gas snapshot
ChefMist May 8, 2024
69eba4a
Merge branch 'main' into feat/tweaks-v2
chefburger Jun 5, 2024
84ffdbe
Merge branch 'main' into feat/tweaks-v2
ChefMist Jun 5, 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
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testBurnNativeCurrency.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
134689
134601
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testFuzzUpdateDynamicLPFee.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
32311
32323
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasBurnHalfBin.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
143755
143645
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasBurnNineBins.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
289889
289801
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
127816
127728
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasDonate.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
116226
116154
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasMintNneBins-1.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
971105
971033
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasMintNneBins-2.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
329671
329599
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasMintOneBin-1.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
339216
339144
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasMintOneBin-2.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
141675
141603
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
174404
174313
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
180277
180186
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
134199
134108
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testMintNativeCurrency.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
305997
305925
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
354338
354266
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
169775
169703
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#addLiquidity_nativeToken.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
240472
240436
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#donateBothTokens.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
165801
165729
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#gasDonateOneToken.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
110199
110163
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#removeLiquidity_toNonEmpty.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
116943
116888
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_againstLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
133733
133642
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
166497
166407
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
153863
153772
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
145886
145795
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#Vault.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
5663
5631
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenAddLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
80269
82697
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenFlashloan.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
119861
119741
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenMultiHopSwap.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
43540
43449
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenSwap.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
43536
43445
20 changes: 13 additions & 7 deletions src/Vault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,18 @@ contract Vault is IVault, VaultToken, Ownable {

/// @inheritdoc IVault
function take(Currency currency, address to, uint256 amount) external override isLocked {
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
currency.transfer(to, amount);
unchecked {
chefburger marked this conversation as resolved.
Show resolved Hide resolved
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

same reason as mint

currency.transfer(to, amount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure whether there are any security issues .
Now we update the reservesOfVault after transfer token.
But after we merge the PR 33(reserveOf transient) , should be fine.

}
}

/// @inheritdoc IVault
function mint(address to, Currency currency, uint256 amount) external override isLocked {
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
_mint(to, currency, amount);
unchecked {
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
_mint(to, currency, amount);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. amount is positive
  2. if amount.toInt128 doesn't revert, then amount is a valid int128
  3. combine 1 & 2, converting from positive to negative will never go overflow

}

function sync(Currency currency) public returns (uint256 balance) {
Expand All @@ -135,9 +139,11 @@ contract Vault is IVault, VaultToken, Ownable {
function settleFor(Currency currency, address target, uint256 amount) external isLocked {
/// @notice settle all outstanding debt if amount is 0
/// It will revert if target has positive delta
if (amount == 0) amount = (-SettlementGuard.getCurrencyDelta(target, currency)).toUint256();
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
SettlementGuard.accountDelta(target, currency, amount.toInt128());
unchecked {
if (amount == 0) amount = (-SettlementGuard.getCurrencyDelta(target, currency)).toUint256();
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
SettlementGuard.accountDelta(target, currency, amount.toInt128());
}
}

Copy link
Collaborator

@chefburger chefburger Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. amount is positive
  2. SettlementGuard.getCurrencyDelta(target, currency) cant be over int128 otherwise it will revert in line 144

/// @inheritdoc IVault
Expand Down
Loading