-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: txn cache roundtrip integration test #46
Conversation
// create a new signed test transaction | ||
let wallet = PrivateKeySigner::random(); | ||
let tx_envelope = new_test_tx(&wallet); | ||
|
||
// send that transaction to ensure there is at least one tx in pool to parse | ||
let _ = client | ||
.post(config.tx_pool_url.to_string() + "/add") | ||
.post(config.tx_pool_url.to_string() + "add") |
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.
The URL construction is missing a forward slash, which could lead to an incorrect endpoint. To ensure proper URL formation, modify the line as follows:
.post(config.tx_pool_url.to_string() + "/add")
This change will correctly append the /add
path to the base URL, regardless of whether the base URL ends with a trailing slash or not.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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.
should i change the way im handling this to add the slash back in the code and remove it from the config?
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 would pull the trailing slash off the base URL and leave it on the /get
and /add
for example.
the integration tests don't actually run locally, i don't know if we're actually running them anywhere? cc maybe @rswanson ? |
|
||
// assert that we parsed at least one transaction | ||
let got = tx_poller::TxPoller::new(&config).check_tx_pool().await.unwrap(); | ||
assert!(!got.is_empty()); |
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.
🎀
// create a new signed test transaction | ||
let wallet = PrivateKeySigner::random(); | ||
let tx_envelope = new_test_tx(&wallet); | ||
|
||
// send that transaction to ensure there is at least one tx in pool to parse | ||
let _ = client | ||
.post(config.tx_pool_url.to_string() + "/add") | ||
.post(config.tx_pool_url.to_string() + "add") |
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 would pull the trailing slash off the base URL and leave it on the /get
and /add
for example.
de4e96a
to
0da23dc
Compare
@dylanlott okay, i updated the code here to keep the slash on and remove it from the config. we'll need to update the infra config too. i can do that now |
0da23dc
to
6adb5e0
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.
LGTM 🎀
No description provided.