Builder client #10703
Builder client #10703
Conversation
|
I tested this with mergemock, they look good. We can merge this to |
| return s.Bytes(), nil | ||
| } | ||
|
|
||
| type Uint64String uint64 |
There was a problem hiding this comment.
Unfortunately this seems to produce 0x01 style hex-encoded output when I just swap the Uint64String types for hexutil.Uint64. I wonder if I'm doing something wrong?
| // sszBytesToUint256 creates a Uint256 from a ssz-style (little-endian byte slice) representation. | ||
| func sszBytesToUint256(b []byte) Uint256 { | ||
| bi := new(big.Int) | ||
| return Uint256{Int: bi.SetBytes(bytesutil.ReverseByteOrder(b))} |
There was a problem hiding this comment.
I'm not so sure about reverse byte order here
There was a problem hiding this comment.
I added a few additional tests to improve confidence in the round trip:
https://github.com/prysmaticlabs/prysm/pull/10703/files#diff-9ed81b08538dbc58eb053d34c85e5425a58aab751d51e785294ea649b12c7020R696-R727
Interested in other ideas for test cases, I have a few more I want to write.
terencechain
added
Ready For Review
| type reqOption func(*http.Request) | ||
|
|
||
| // do is a generic, opinionated GET function to reduce boilerplate amongst the getters in this packageapi/client/builder/types.go. | ||
| func (c *Client) do(ctx context.Context, method string, path string, body io.Reader, opts ...reqOption) ([]byte, error) { |
There was a problem hiding this comment.
I recall seeing something similar when you were writing a beacon client. I wonder if there's room for unifying these so that next time we have to write a client, we don't have to write most of this boilerplate for request and response handling
There was a problem hiding this comment.
Agree. This version is a slight iteration on the previous one, would like to circle back and unify them but wanted to limit scope of this change.
| body, err := json.Marshal(v) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "error encoding the SignedBlindedBeaconBlockBellatrix value body in SubmitBlindedBlock") | ||
| } |
There was a problem hiding this comment.
Is it preferred to do something like:
json.NewEncoder(w).Encode(dataToWrite)which directly uses the http response writer variable as the target for the encoding? I think it could reduce some duplicated code across this file, same with unmarshaling
There was a problem hiding this comment.
How would that reduce duplicated code? Are you suggesting we could change the signature of do to take an interface and do the encoding there instead? like:
func (c *Client) do(ctx context.Context, method string, path string, body interface{}, opts ...reqOption) ([]byte, error) {
rb := bytes.NewBuffer(nil)
json.NewEncoder(rb).Encode(body)
req, err := http.NewRequestWithContext(ctx, method, u.String(), io.NopCloser(rb))
...
}
What type of PR is this?
(branch-to-branch comparison)
Feature
What does this PR do? Why is it needed?
Adds a client for the builder api, described at https://ethereum.github.io/builder-specs/#/Builder
Which issues(s) does this PR fix?
This covers the "Implement builder APIs" Requirements at #10593
Other notes for review