- 
                Notifications
    You must be signed in to change notification settings 
- Fork 139
expose eligible_at and signed_at in data api #359
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
base: main
Are you sure you want to change the base?
Conversation
| Interesting, thanks! Reminds me a bit of #260 Will review in depth soon | 
| Codecov Report
 ❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@             Coverage Diff             @@
##             main     #359       +/-   ##
===========================================
+ Coverage   19.18%   30.96%   +11.77%     
===========================================
  Files          20       24        +4     
  Lines        3722     4819     +1097     
===========================================
+ Hits          714     1492      +778     
- Misses       2915     3125      +210     
- Partials       93      202      +109     
 Flags with carried forward coverage won't be shown. Click here to find out more. 
 | 
| yeah #260 was definitely a partial inspiration for some of the structure/implementation | 
| @metachris wanted to see if you've had a chance to take a look at this | 
| require.Equal(t, http.StatusOK, rr.Code) | ||
| }) | ||
|  | ||
| t.Run("Accept valid slot", func(t *testing.T) { | 
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.
duplicate test?
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.
yep good catch
| } | ||
| } | ||
|  | ||
| func DeliveredPayloadEntryToBidTraceV3JSON(payload *DeliveredPayloadEntry) common.BidTraceV3JSON { | 
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.
it'll be great if there's tests for these for json marshal and unmarshalling
| Asking for a bit of patience, as we're working on other important PRs to get merged first (in particular #380) | 
| @rajivpo could you please rebase? | 
| yes will get this done shortly | 
| something went wrong with this rebase, it now has 45 commits and a ton of changes 🤔 | 
18524f1    to
    6a4f5da      
    Compare
  
    
📝 Summary
Exposes
eligible_atandsigned_atvia data api⛱ Motivation and Context
The two relevant timestamps were captured and added in #301. For research purposes, it would be convenient to expose these as part of
builder_blocks_receivedandproposer_payload_delivered. I have opted for providing the millisecond timestamps as milliseconds should be the roughly the relevant timeframe for sim and is aligned with the existingtimestamp_msfield inBidTraceV2WithTimestampJSON. Additionally, tests for/relay/v1/data/bidtraces/builder_blocks_receivedhave been added.This is definitely not the cleanest implementation, so any and all feedback is appreciated!
📚 References
#301
#260 (edited to include)
✅ I have run these commands
make lintmake test-racego mod tidyCONTRIBUTING.md