Add payload_size to ExplorerTransaction impl
#2433
Open
+58
−27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `hotshot-query-service` has an updated requirement for the trait `ExplorerTransaction` which needs to be implemented. The primary reason for this new method is to correctly reflect the actual transaction size, as it currently reflects the size of the entire block, instead of the size of the transaction itself. In order for this to be adjusted the `ExplorerTransaction` trait within the `hotshot-query-service` library has been modified to require the `payload_size` method so the transaction is able to specify its own size. This change implements the new method by passing the method call processing down to the underlying payload, which should reflect the actual size of the transaction itself.
Ayiga
requested review from
philippecamacho,
ImJeremyHe,
sveitser,
jbearer,
tbro and
imabdulbasit
as code owners
The `TestConfig` `new` function references two hard-coded values related to the version of the sequencer to run. These values have been hard coded to `2` and `3` to indicate the versions of the sequencer that we have been developing with. However, the version `3` and the logic that ensues from it is based on the assumption that version `3` will be the marketplace, which is the value it has had historically up to this point. Because of the change of this version's number, and the errors that will result when we assign some other version of the sequencer to `3`, We've opted to instead reference the static version values of the allocated values for each of the versions for this transition. This should make it clear, and accurate, of which sequencer version we're referencing, and at which point they should change.
There are a few potential issues of note with the `wait_for_service` function. This function is intended to check if a service is ready by ensuring its health check passes before interacting with it. * The first potential issue is the URL construction. The string `/healthcheck` is appended to the end of the given URL. This assumes that the URL given does not have a path at all. * The second potential issue has to do with how we check for a "success". The success is determined by receiving any response back from the server. This means that if the server returns an error code, such as `404`, the health check would still pass. Normally health checks must ensure that a 200 status code is returned. To fix these potential issues we addressed them directly. First he URL construction was modified to use the `Url::join` method, as it is aware of relative and absolute path references. Second the reqwest call was re-written to ensure that the response retrieved is an Ok. This didn't have a direct chagne to the logic, but helped with the readability of the next portion, and it ensures that the `response` variable persists beyond a single closure. Third the response's status code is checked with the `http::status::StatusCode::is_success` method. Finally, some comments were added to provide a simple explanation of what the `wait_for_service` function is doing, and some behavior implications. Beyond these changes, no other behavior modifications have been made. This means that the health check still checks to see of the `Response` is able to retrieve a textual representation of the response.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2432
This PR:
Updates the dependency on the
hotshot-query-serviceto one that has some error handling fixes for the Block Explorer, and one that has an update to theExplorerTransactiontrait.Updates the implementation of the
ExplorerTransactiontrait to implement the newpayload_sizemethod.The updated
hotshot-query-servicedependency should also correct how many transactions are pulled when querying a block with more than the requested limit of transactions.This PR does not:
Target a new explicit version tag of the
hotshot-query-service, but instead targets a subtag:v0.1.75-explorer-fixes.