Skip to content

Commit

Permalink
Don't track pending deposits in AbstractTBTCDepositor contract
Browse files Browse the repository at this point in the history
To optimize gas usage we removed `pendingDeposits` mapping. This
operation reduces gas usage of `_initializeDeposit` function by ~20k
gas.

After this change it is implementation's contract responsibility to
ensure `_finalizeDeposit` function cannot be called twice for the same
deposit.

`_initializeDeposit` function is not affected as the bridge blocks
possibility of revealing the same deposit twice.
  • Loading branch information
nkuba committed Feb 8, 2024
1 parent 7561ba7 commit 69f52c2
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 37 deletions.
27 changes: 10 additions & 17 deletions solidity/contracts/integrator/AbstractTBTCDepositor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ import "./ITBTCVault.sol";
/// }
///
/// function finalizeProcess(uint256 depositKey) external {
/// // Ensure the function cannot be called for the same deposit
/// // twice.
///
/// (
/// uint256 initialDepositAmount,
/// uint256 tbtcAmount,
Expand All @@ -84,11 +87,6 @@ abstract contract AbstractTBTCDepositor {
IBridge public bridge;
/// @notice TBTCVault contract address.
ITBTCVault public tbtcVault;
/// @notice Mapping holding information about pending deposits that have
/// been initialized but not finalized yet. If the deposit is not
/// in this mapping it means it has already been finalized or it
/// has not been initialized yet.
mapping(uint256 => bool) public pendingDeposits;

// Reserved storage space that allows adding more variables without affecting
// the storage layout of the child contracts. The convention from OpenZeppelin
Expand Down Expand Up @@ -137,6 +135,8 @@ abstract contract AbstractTBTCDepositor {
/// - The revealed vault address must match the TBTCVault address,
/// - All requirements from {Bridge#revealDepositWithExtraData}
/// function must be met.
/// @dev This function doesn't validate if a deposit has been initialized before,
/// as the Bridge won't allow the same deposit to be revealed twice.
// slither-disable-next-line dead-code
function _initializeDeposit(
IBridgeTypes.BitcoinTxInfo calldata fundingTx,
Expand All @@ -150,8 +150,6 @@ abstract contract AbstractTBTCDepositor {
reveal.fundingOutputIndex
);

pendingDeposits[depositKey] = true;

emit DepositInitialized(
depositKey,
/* solhint-disable-next-line not-rely-on-time */
Expand Down Expand Up @@ -180,6 +178,9 @@ abstract contract AbstractTBTCDepositor {
/// (in the context of this contract) yet.
/// - The deposit must be finalized on the Bridge side. That means the
/// deposit must be either swept or optimistically minted.
/// @dev This function doesn't validate if a deposit has been finalized before,
/// it is a responsibility of the implementing contract to ensure this
/// function won't be called twice for the same deposit.
/// @dev IMPORTANT NOTE: The tbtcAmount returned by this function is an
/// approximation. See documentation of the `calculateTbtcAmount`
/// responsible for calculating this value for more details.
Expand All @@ -192,11 +193,11 @@ abstract contract AbstractTBTCDepositor {
bytes32 extraData
)
{
require(pendingDeposits[depositKey], "Deposit not initialized");

IBridgeTypes.DepositRequest memory deposit = bridge.deposits(
depositKey
);
require(deposit.revealedAt != 0, "Deposit not initialized");

(, uint64 finalizedAt) = tbtcVault.optimisticMintingRequests(
depositKey
);
Expand All @@ -206,14 +207,6 @@ abstract contract AbstractTBTCDepositor {
"Deposit not finalized by the bridge"
);

// We can safely delete the deposit from the pending deposits mapping.
// This deposit cannot be initialized again because the bridge does not
// allow to reveal the same deposit twice. Deleting the deposit from
// the mapping will also prevent the finalizeDeposit function from
// being called again for the same deposit.
// slither-disable-next-line reentrancy-no-eth
delete pendingDeposits[depositKey];

initialDepositAmount = deposit.amount * SATOSHI_MULTIPLIER;

tbtcAmount = _calculateTbtcAmount(deposit.amount, deposit.treasuryFee);
Expand Down
22 changes: 2 additions & 20 deletions solidity/test/integrator/AbstractTBTCDepositor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,6 @@ describe("AbstractTBTCDepositor", () => {
.withArgs(fixture.expectedDepositKey)
})

it("should store the deposit as pending", async () => {
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
expect(await depositor.pendingDeposits(fixture.expectedDepositKey)).to
.be.true
})

it("should emit the DepositInitialized event", async () => {
await expect(tx)
.to.emit(depositor, "DepositInitialized")
Expand Down Expand Up @@ -180,10 +174,10 @@ describe("AbstractTBTCDepositor", () => {
await restoreSnapshot()
})

it("should revert", async () => {
it("should not revert", async () => {
await expect(
depositor.finalizeDepositPublic(fixture.expectedDepositKey)
).to.be.revertedWith("Deposit not initialized")
).to.be.not.reverted
})
})

Expand Down Expand Up @@ -239,12 +233,6 @@ describe("AbstractTBTCDepositor", () => {
await restoreSnapshot()
})

it("should remove the deposit from pending", async () => {
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
expect(await depositor.pendingDeposits(fixture.expectedDepositKey))
.to.be.false
})

it("should emit the DepositFinalized event", async () => {
await expect(tx)
.to.emit(depositor, "DepositFinalized")
Expand Down Expand Up @@ -289,12 +277,6 @@ describe("AbstractTBTCDepositor", () => {
await restoreSnapshot()
})

it("should remove the deposit from pending", async () => {
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
expect(await depositor.pendingDeposits(fixture.expectedDepositKey))
.to.be.false
})

it("should emit the DepositFinalized event", async () => {
await expect(tx)
.to.emit(depositor, "DepositFinalized")
Expand Down

0 comments on commit 69f52c2

Please sign in to comment.