- 
                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)