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

Fix event emission of AToken and DebtToken events #682

Merged
merged 19 commits into from
Nov 15, 2022
Merged

Fix event emission of AToken and DebtToken events #682

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
251c052
fix: Initial fix for emitting whole balance in Transfer events
miguelmtzinf May 23, 2022
b64ec22
fix: Initial fix for emitting accumulating Mint events in transfers
miguelmtzinf May 23, 2022
a31a16a
fix: Fix the amount of BalanceTransfer event
miguelmtzinf May 23, 2022
617facd
test: Add tests for event emission in tokens
miguelmtzinf Jun 1, 2022
4ae5648
fix: Refactor old test cases for AToken events
miguelmtzinf Jun 1, 2022
449a28c
fix: Fix errors on tests
miguelmtzinf Jun 1, 2022
ee58f15
test: Add failing tests of AToken events due to onBehalfOf mechanism
miguelmtzinf Jun 1, 2022
8d0af95
fix: Fix tests
miguelmtzinf Jun 1, 2022
e353c29
fix: Fix order of emission of events in AToken ttransfer
miguelmtzinf Jun 1, 2022
7d22082
fix: Enhance natspec docs of main events
miguelmtzinf Jun 1, 2022
80c6e5d
fix: Rename `amount` field in IStableDebtToken to `value`
miguelmtzinf Jun 1, 2022
86aff28
fix: Add some little refactor to tests
miguelmtzinf Jun 2, 2022
2b4f336
fix: Add emission of Transfer events for accruals in transfer function
miguelmtzinf Jun 13, 2022
89eca28
fix: Fix caller of Mint event when transferFrom
miguelmtzinf Jun 13, 2022
a85c48b
docs: Fix missing docs in burnScaled function
miguelmtzinf Jun 20, 2022
8152611
fix: Add more docs to Burn event
miguelmtzinf Aug 9, 2022
629d887
Merge branch 'feat/3.0.1' into fix/681-atoken-events
miguelmtzinf Nov 2, 2022
c54ce21
fix: Resolve merge error in tests
miguelmtzinf Nov 2, 2022
b9ad0be
fix: Revert changes on event param names of StableDebtToken
miguelmtzinf Nov 3, 2022
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 contracts/interfaces/IAToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ interface IAToken is IERC20, IScaledBalanceToken, IInitializableAToken {
* @dev Emitted during the transfer action
* @param from The user whose tokens are being transferred
* @param to The recipient
* @param value The amount being transferred
* @param value The scaled amount being transferred
* @param index The next liquidity index of the reserve
**/
event BalanceTransfer(address indexed from, address indexed to, uint256 value, uint256 index);
Expand Down
11 changes: 6 additions & 5 deletions contracts/interfaces/IScaledBalanceToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ pragma solidity 0.8.10;
/**
* @title IScaledBalanceToken
* @author Aave
* @notice Defines the basic interface for a scaledbalance token.
* @notice Defines the basic interface for a scaled-balance token.
**/
interface IScaledBalanceToken {
/**
* @dev Emitted after the mint action
* @param caller The address performing the mint
* @param onBehalfOf The address of the user that will receive the minted scaled balance tokens
* @param value The amount being minted (user entered amount + balance increase from interest)
* @param balanceIncrease The increase in balance since the last action of the user
* @param value The scaled amount being minted (based on user entered amount and balance increase from interest)
* @param balanceIncrease The increase in scaled balance since the last action of 'onBehalfOf'
* @param index The next liquidity index of the reserve
**/
event Mint(
Expand All @@ -25,10 +25,11 @@ interface IScaledBalanceToken {

/**
* @dev Emitted after scaled balance tokens are burned
* @dev If the burn function does not involve a transfer of the underlying asset, the target defaults to zero address
* @param from The address from which the scaled tokens will be burned
* @param target The address that will receive the underlying, if any
miguelmtzinf marked this conversation as resolved.
Show resolved Hide resolved
* @param value The amount being burned (user entered amount - balance increase from interest)
* @param balanceIncrease The increase in balance since the last action of the user
* @param value The scaled amount being burned (user entered amount - balance increase from interest)
* @param balanceIncrease The increase in scaled balance since the last action of 'from'
* @param index The next liquidity index of the reserve
**/
event Burn(
Expand Down
16 changes: 8 additions & 8 deletions contracts/interfaces/IStableDebtToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,17 @@ interface IStableDebtToken is IInitializableDebtToken {
* @dev Emitted when new stable debt is minted
* @param user The address of the user who triggered the minting
* @param onBehalfOf The recipient of stable debt tokens
* @param amount The amount minted (user entered amount + balance increase from interest)
* @param currentBalance The current balance of the user
* @param balanceIncrease The increase in balance since the last action of the user
* @param value The amount minted (user entered amount + balance increase from interest)
* @param currentBalance The balance of the user based on the previous balance and balance increase from interest
miguelmtzinf marked this conversation as resolved.
Show resolved Hide resolved
* @param balanceIncrease The increase in balance since the last action of the user 'onBehalfOf'
* @param newRate The rate of the debt after the minting
* @param avgStableRate The next average stable rate after the minting
* @param newTotalSupply The next total supply of the stable debt token after the action
**/
event Mint(
address indexed user,
address indexed onBehalfOf,
uint256 amount,
uint256 value,
miguelmtzinf marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing this parameter name will likely break the subgraph

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The subgraph will need to differentiate pre and post patch versions

Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem reasonable only due to a name change. Unless the subgraph breaks anyway or there is some major reason

Copy link
Contributor Author

@miguelmtzinf miguelmtzinf Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The subgraph will need to differentiate either way. However, this change will affect the subgraph logic so It may be interesting to revert it back. Thoughts @defispartan ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Last month we implemented a fix for the current V3 subgraphs, so now these subgraphs correctly handle mint and burn events. With this change we would need to add versioning throughout each V3 subgraph to say:

if(blockNumber > ...){
	// use updated mint/burn logic
	// use updated naming: event.params.value
} else {
	// use current mint/burn logic
	// use current naming: event.params.amount
}

Since we have a working version already and handling this would be quite ugly, I would be in favor of reverting the event change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. Reverting the change in order to make the subgraph's logic less painful

uint256 currentBalance,
uint256 balanceIncrease,
uint256 newRate,
Expand All @@ -35,15 +35,15 @@ interface IStableDebtToken is IInitializableDebtToken {
/**
* @dev Emitted when new stable debt is burned
* @param from The address from which the debt will be burned
* @param amount The amount being burned (user entered amount - balance increase from interest)
* @param currentBalance The current balance of the user
* @param balanceIncrease The the increase in balance since the last action of the user
* @param value The amount being burned (user entered amount - balance increase from interest)
* @param currentBalance The balance of the user based on the previous balance and balance increase from interest
* @param balanceIncrease The increase in balance since the last action of 'from'
* @param avgStableRate The next average stable rate after the burning
* @param newTotalSupply The next total supply of the stable debt token after the action
**/
event Burn(
address indexed from,
miguelmtzinf marked this conversation as resolved.
Show resolved Hide resolved
uint256 amount,
uint256 value,
miguelmtzinf marked this conversation as resolved.
Show resolved Hide resolved
uint256 currentBalance,
uint256 balanceIncrease,
uint256 avgStableRate,
Expand Down
6 changes: 2 additions & 4 deletions contracts/protocol/tokenization/AToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,6 @@ contract AToken is VersionedInitializable, ScaledBalanceTokenBase, EIP712Base, I
// Being a normal transfer, the Transfer() and BalanceTransfer() are emitted
// so no need to emit a specific event here
_transfer(from, to, value, false);

emit Transfer(from, to, value);
}

/// @inheritdoc IERC20
Expand Down Expand Up @@ -216,13 +214,13 @@ contract AToken is VersionedInitializable, ScaledBalanceTokenBase, EIP712Base, I
uint256 fromBalanceBefore = super.balanceOf(from).rayMul(index);
uint256 toBalanceBefore = super.balanceOf(to).rayMul(index);

super._transfer(from, to, amount.rayDiv(index).toUint128());
super._transfer(from, to, amount, index);

if (validate) {
POOL.finalizeTransfer(underlyingAsset, from, to, amount, fromBalanceBefore, toBalanceBefore);
}

emit BalanceTransfer(from, to, amount, index);
emit BalanceTransfer(from, to, amount.rayDiv(index), index);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add amount as another field to this event? It won't be used for anything, but this way the subgraph can differentiate which logic to use based on a different event signature.

The alternative is that we'd need to store the exact block that each market was updated to 3.0.1, and decide which logic to use based on the market and blockNumber.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not make much sense to add another param if its not needed tbh.

Are not we using blockNumber already to differentiate? I think there is no other way

Copy link
Contributor

Choose a reason for hiding this comment

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

No, there's nowhere else in the subgraphs where events need to be differentiated between contract versions using block number. It's extremely ugly, and prone to errors if tokens have events in this same block as the update, or if not all tokens are updated in the exact same block

}

/**
Expand Down
1 change: 0 additions & 1 deletion contracts/protocol/tokenization/base/IncentivizedERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,6 @@ abstract contract IncentivizedERC20 is Context, IERC20Detailed {
incentivesControllerLocal.handleAction(recipient, currentTotalSupply, oldRecipientBalance);
}
}
emit Transfer(sender, recipient, amount);
}

/**
Expand Down
39 changes: 39 additions & 0 deletions contracts/protocol/tokenization/base/ScaledBalanceTokenBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ abstract contract ScaledBalanceTokenBase is MintableIncentivizedERC20, IScaledBa
* @dev In some instances, a burn transaction will emit a mint event
* if the amount to burn is less than the interest that the user accrued
* @param user The user which debt is burnt
* @param target The address that will receive the underlying, if any
* @param amount The amount getting burned
* @param index The variable debt index of the reserve
**/
Expand Down Expand Up @@ -125,4 +126,42 @@ abstract contract ScaledBalanceTokenBase is MintableIncentivizedERC20, IScaledBa
emit Burn(user, target, amountToBurn, balanceIncrease, index);
}
}

/**
* @notice Implements the basic logic to transfer scaled balance tokens between two users
* @dev It emits a mint event with the interest accrued per user
* @param sender The source address
* @param recipient The destination address
* @param amount The amount getting transferred
* @param index The next liquidity index of the reserve
**/
function _transfer(
address sender,
address recipient,
uint256 amount,
uint256 index
) internal {
uint256 senderScaledBalance = super.balanceOf(sender);
uint256 senderBalanceIncrease = senderScaledBalance.rayMul(index) -
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably better to simplify the operation to only do 1 rayMul, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but we are using this approach on other parts throughout the code

senderScaledBalance.rayMul(_userState[sender].additionalData);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@miguelmtzinf Given that this code operates on _userState[sender] (or recipient) after and being 1 slot packed with additionalData, is not more gas-efficient to do a

UserState memory senderState = _userState[sender];

and then just operate with the fields in memory?
Unless the compiler takes this into account already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not worth to do what you suggest because we need to update storage sender/recipient 's state later on


uint256 recipientScaledBalance = super.balanceOf(recipient);
uint256 recipientBalanceIncrease = recipientScaledBalance.rayMul(index) -
recipientScaledBalance.rayMul(_userState[recipient].additionalData);

_userState[sender].additionalData = index.toUint128();
_userState[recipient].additionalData = index.toUint128();

super._transfer(sender, recipient, amount.rayDiv(index).toUint128());

emit Transfer(address(0), sender, senderBalanceIncrease);
emit Mint(_msgSender(), sender, senderBalanceIncrease, senderBalanceIncrease, index);

if (recipientBalanceIncrease > 0) {
emit Transfer(address(0), recipient, recipientBalanceIncrease);
emit Mint(_msgSender(), recipient, recipientBalanceIncrease, recipientBalanceIncrease, index);
}

emit Transfer(sender, recipient, amount);
}
}
Loading