- 
                Notifications
    You must be signed in to change notification settings 
- Fork 24
feat: simulate bundles in ingress-rpc #46
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
707977e    to
    ba5501e      
    Compare
  
    | pub struct BundleWithMetadata { | ||
| bundle: Bundle, | ||
| uuid: Uuid, | ||
| transactions: Vec<OpTxEnvelope>, | ||
| meter_bundle_response: Option<MeterBundleResponse>, | ||
| } | 
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 a Bundle fails validate_bundle then there wouldn't be a time to meter bundle, which is why I wrapped MeterBundleResponse around an Option
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'm not sure I understand what BundleWithMetadata is supposed to be. It seems to have two conflicting purposes: 1) transform the bundle data into a more useful format, and 2) provide all the information the bundler needs to receive over Kafka. For (1), it's providing the transaction data twice: once as bytes and once as parsed data structures.
But when I dig into the Kafka publishing code, it looks like BundleWithMetadata is never sent over the wire? So is adding it here actually useful? I think we need a type for the actual Kafka event, which would just have a uuid, a bundle, and a meter_bundle_response. In that type, meter_bundle_response wouldn't be optional.
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.
This looks good now, but for context, I'm going to have to stop using BundleWithMetadata in node-reth's metering crate since I won't have the meter_bundle_response yet. If there ends up being another type with the utility functions on it (I'm using bundle_with_metadata.bundle_hash()) then I'll use it, otherwise I'll reintroduce a standalone function for it local to that crate.
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'm using bundle_with_metadata.bundle_hash()
if that's the case, perhaps in node-reth it can remain as Bundle ?
since as we discussed offline that i believe the intention with BundleWithMetadata is the struct to be sent to kafka with things like the metering results
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.
Yes, the most straightforward thing to do is to never convert it from Bundle at all. But it'd be nice to have a way to pull in utility functions from tips-core. Right now they're all BundleWithMetadata methods, so we'd either want to move them to a new type (in that hypothetical third PR we talked about), change them to standalone utility functions instead of methods, or just reimplement them where BundleWithMetadata doesn't fit.
| pub struct BundleWithMetadata { | ||
| bundle: Bundle, | ||
| uuid: Uuid, | ||
| transactions: Vec<OpTxEnvelope>, | ||
| meter_bundle_response: Option<MeterBundleResponse>, | ||
| } | 
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'm not sure I understand what BundleWithMetadata is supposed to be. It seems to have two conflicting purposes: 1) transform the bundle data into a more useful format, and 2) provide all the information the bundler needs to receive over Kafka. For (1), it's providing the transaction data twice: once as bytes and once as parsed data structures.
But when I dig into the Kafka publishing code, it looks like BundleWithMetadata is never sent over the wire? So is adding it here actually useful? I think we need a type for the actual Kafka event, which would just have a uuid, a bundle, and a meter_bundle_response. In that type, meter_bundle_response wouldn't be optional.
64f8522    to
    6027a50      
    Compare
  
            
          
                crates/core/src/types.rs
              
                Outdated
          
        
      | } | ||
|  | ||
| impl Default for MeterBundleResponse { | ||
| fn default() -> Self { | 
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.
This seems only appropriate for use in tests, so I'm not sure if the Default trait approach is the right way to go. I think it'll lead people to populate their bundles with default MeterBundleResponses, which the builder will need to log errors for since they won't be able to be metered. Feel free to leave it as is, but I think I'd change this to a function in the tests (which is admittedly a bit harder for tests defined in an inner module).
Overview
This PR simulates bundles on
eth_sendBundleand insendRawTxand for now, rejects a request if the bundle simulation took over 2 seconds.TODO: