-
Notifications
You must be signed in to change notification settings - Fork 75
feat: add metadata event emitter contract #1138
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
Conversation
Signed-off-by: amateima <amatei@umaproject.org>
Signed-off-by: amateima <amatei@umaproject.org>
8f36a7a to
f88ed47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two quick comments!
contracts/AcrossEventEmitter.sol
Outdated
| /** | ||
| * @notice Prevents native token from being sent to this contract | ||
| */ | ||
| receive() external payable { | ||
| revert("Contract does not accept native token"); | ||
| } | ||
|
|
||
| /** | ||
| * @notice Prevents native token from being sent to this contract via fallback | ||
| */ | ||
| fallback() external payable { | ||
| revert("Contract doesn't accept native token"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why implement these functions at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered would be good practice to not allow funds being sent by mistake, but I can remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't implement them, they'll revert when users try to call them, so it would be the same outcome.
contracts/AcrossEventEmitter.sol
Outdated
| require(data.length > 0, "Data cannot be empty"); | ||
| require(data.length <= 2048, "Data too large"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why limit the size of the data?
Would this protect us from anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the upper limit here, but I'd keep the > 0 validation, so that the indexer would not index events with no metadata at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Signed-off-by: amateima <amatei@umaproject.org>
e934edd to
c7c0175
Compare
contracts/AcrossEventEmitter.sol
Outdated
| * @notice Emits metadata as an event | ||
| * @param data The bytes data to emit | ||
| */ | ||
| function emitData(bytes calldata data) external nonReentrant { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-entrancy guards are generally best practice, but you don't need them if your contract isn't making any external calls (which is the case in this contract).
I would suggest removing to save a little gas.
This PR introduces a new
AcrossEventEmittercontract that provides a dedicated mechanism for emitting on-chain metadata events.Changes
AcrossEventEmitteris a simple contract for emitting arbitrary bytes-encoded metadata as on-chain events. The usecase behind this contract is to emit metadata without modifying Across core contracts. This will allow the indexer to enrich Across protocol data with additional info that cannot be grabbed easily from onchain events (e.g swaps performed as destination actions)