Skip to content
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

PRO-1620: Ensure that default port is used if none is given in configuration #5281

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

MxmUrw
Copy link
Contributor

@MxmUrw MxmUrw commented Sep 19, 2024

Pull Request

Closes: PRO-1620

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have written sufficient tests.
  • I have written and tested required migrations.
  • I have updated documentation where appropriate.

Summary

Currently the polkadot http rpc client uses jsonrpsee's HttpClient. Unfortunately, since we are on version 0.16 of jsonrpsee, this client has the rule that a port must always be given, even if it should be the default port. Later versions of jsonrpsee forgo this requirement.

To make sure that the user configures a valid http endpoint for polkadot, we currently check that the url contains a port. Unfortunately the checking logic uses a regex and accepts urls such as https://endpoint/path:port as valid, even though it shouldn't.

It also seems that the Solana configuration logic mistakenly was copied from Polkadot since it calls the same validation logic, without there being a need for that.

This PR does multiple things:

  • Instead of requiring the user to always give a port for polkadot endpoints, we automatically insert the default port in case none was given.
  • Remove tests which check that polkadot endpoints always contain ports.
  • Remove the port validation logic for solana.

Non-Breaking changes

If this PR includes non-breaking changes, select the non-breaking label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 84.31373% with 8 lines in your changes missing coverage. Please review.

Project coverage is 70%. Comparing base (68ab19b) to head (c4d37bb).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
engine/src/dot/http_rpc.rs 84% 3 Missing and 5 partials ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #5281    +/-   ##
======================================
  Coverage     70%     70%            
======================================
  Files        487     487            
  Lines      86802   87138   +336     
  Branches   86802   87138   +336     
======================================
+ Hits       60807   61118   +311     
- Misses     22662   22740    +78     
+ Partials    3333    3280    -53     
Flag Coverage Δ
70% <84%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MxmUrw MxmUrw changed the title Add ensure_port() to be called before creating HttpClient for polka… PRO-1620: Ensure that default port is used if none is given in configuration Sep 19, 2024
@MxmUrw MxmUrw marked this pull request as ready for review September 19, 2024 15:14
@MxmUrw MxmUrw requested a review from kylezs as a code owner September 19, 2024 15:14
It seems that this was added accidentally. Solana uses `reqwest` for
api calls, which should deal fine without ports.

Also remove test which checks that polkadot endpoints contain ports,
since this is no longer required.
engine/src/dot/http_rpc.rs Outdated Show resolved Hide resolved
engine/src/dot/http_rpc.rs Outdated Show resolved Hide resolved
engine/src/dot/http_rpc.rs Outdated Show resolved Hide resolved
@MxmUrw MxmUrw requested a review from kylezs September 23, 2024 08:39
@MxmUrw MxmUrw added this pull request to the merge queue Sep 23, 2024
Merged via the queue into main with commit ce2f615 Sep 23, 2024
48 checks passed
@MxmUrw MxmUrw deleted the fix/validate-port-in-configuration-correctly branch September 23, 2024 10:07
syan095 added a commit that referenced this pull request Oct 2, 2024
…-sdk-1.15.2

* origin/main: (31 commits)
  feat: liveness electoral system (#5278)
  chore: bump asset-balances to match release (#5305)
  refactor: Strongly-typed identifiers for SwapId/SwapRequestId (#5294)
  fix: only install solana if `run-job` is true 🐛 (#5304)
  Chore/debug 3 nodes (#5302)
  feat: remove swap and retry batch on price impact (#5285)
  refactor: Collect settings extrinsic in threshold signer pallet into a single extrinsic (#5299)
  feat: add solana monitoring data (#5277)
  fix: Redemption amount printed not consistent in rounding (#5296)
  refactor: use chainflip api for DCA test (#5289)
  fix: add audit exception for RUSTSEC-2024-0375 (#5303)
  fix: filter out stale bitcoin utxos (#5291)
  PRO-1594: Add healthcheck endpoints to broker and lp apis (#5282)
  Tests elections pallet (#5190)
  PRO-1620: Ensure that default port is used if none is given in configuration (#5281)
  test: egress success tests (#5288)
  feat: localnet scripts to create and recreate easier (#5284)
  Denote broadcast timeout in target chain blocks. (#5270)
  fix: force version bump endpoint (#5280)
  refactor: user friendly bouncer new swap cmd  (#5273)
  ...

# Conflicts:
#	.zepter.yaml
#	Cargo.lock
#	api/bin/chainflip-cli/Cargo.toml
#	api/bin/chainflip-lp-api/src/main.rs
#	engine/Cargo.toml
#	state-chain/custom-rpc/Cargo.toml
#	state-chain/custom-rpc/src/lib.rs
#	state-chain/pallets/cf-swapping/src/lib.rs
#	state-chain/runtime/src/monitoring_apis.rs
#	utilities/Cargo.toml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants