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

feat: API Bins check SC compatibility #4342

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Conversation

j4m1ef0rd
Copy link
Contributor

Pull Request

Closes: PRO-972

Checklist

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

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

Added the existing version check to the API lib. This means the Broker, LP API and CLI will all return an error when trying to do a command on the SC using an older version then what's on chain.

  • Version check is on all 3 bins, not just the CLI. Let me know if this is a bad idea.
  • The check uses the API package version, NOT the individual bins version. Is this what we want? I think so.
    • Updated the API version to 1.1.0, like the others. We will need to make sure to update this version when we update the others.
  • The error message does not let the user know they need to download an update or how to do that. This is because the message general, the CFE uses it.
    • Added block number to the error message.
Error: This version '0.1.0' is incompatible with the current release '1.1.0' at block 2117: 0xebd4e5c9c477360da764b20bcc4c0e61c723e6641aa422ebce9d0a07120598ae.

@j4m1ef0rd j4m1ef0rd self-assigned this Dec 12, 2023
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (e8fd491) 71% compared to head (74dd5b1) 71%.
Report is 9 commits behind head on main.

Files Patch % Lines
engine/src/state_chain_observer/client/mod.rs 0% 12 Missing ⚠️
api/lib/src/lib.rs 0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4342    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        390     393     +3     
  Lines      66407   66671   +264     
  Branches   66407   66671   +264     
======================================
+ Hits       47451   47498    +47     
- Misses     16580   16747   +167     
- Partials    2376    2426    +50     

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

@dandanlen
Copy link
Collaborator

@ahasna FYI the api/lib/Cargo.toml will need to be added to the new CI checks.

@dandanlen dandanlen merged commit 4e79432 into main Dec 14, 2023
42 checks passed
@dandanlen dandanlen deleted the feat/cli-check-version-compat branch December 14, 2023 13:23
syan095 added a commit that referenced this pull request Dec 18, 2023
…-timeout

* origin/main:
  feat: end to end network upgrade github action (#4274)
  feat: spec_version of PR is greater than spec version of current release (#4355)
  feat: enforce version is greater than release version on PRs to main (#4351)
  fix: btc witnesser test failing sometimes (#4353)
  fix: connections can become stale when reconnecting (#4310)
  chore: add `chainflip-rpc-node` systemd file 🚀 (#4352)
  feat: API Bins check SC compatibility (#4342)
  chore: update runtime spec checks ⛓️ (#4349)
  feat: Add version cmd to all bins (#4343)
  fix: changelog check 🤫 (#4348)
  chore: update docker tags 🐳 (#4347)
  chore: add runtime version check 👀 (#4344)
  feat: shave fees on ingress (#4335)
  pick/persa fixes (#4329)
  feat: track btc fees on success (#4334)
  ensure we dont create BTC transaction outputs below the bitcoin dust limit (#4340)
  fix: sweeping before withdrawal (#4337)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants