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(trade): Block users from trading with an old app version #2174
feat(trade): Block users from trading with an old app version #2174
Conversation
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.
Looks good overall, but I left some suggestions.
coordinator/migrations/2024-03-06-1135500_add_version_to_user/down.sql
Outdated
Show resolved
Hide resolved
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.
Thanks for going through my review.
I opted to use the Cargo.toml version instead of the pubspec.yaml version as it is easier to access from rust and should probably anyways be bumped with every release. A minor downside of this is now that we have to different sources for the version. However, both versions are bumped by the same ci pipeline during `draft_new_release` - so eventually the source is the same.
We might want to consider simply bumping the versions of all crates instead of selectively bumping some.
13ab779
to
a7e479b
Compare
a7e479b
to
bb70acb
Compare
- name: Bump mobile/native Cargo.toml version | ||
uses: stellar/binaries@v13 | ||
with: | ||
name: cargo-edit | ||
version: 0.11.6 |
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.
❌ This step already exists a few lines above
- id: set-mobile-version | ||
continue-on-error: true | ||
run: | | ||
cargo set-version --package mobile/native ${{ github.event.inputs.version }} |
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.
❌ At least locally this does not work.
Correct would be
cargo set-version --package mobile/native ${{ github.event.inputs.version }} | |
cargo set-version --package native ${{ github.event.inputs.version }} |
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 tested it locally with a different command. Thanks for the fix 🙂
I opted to use the Cargo.toml version instead of the pubspec.yaml version as it is easier to access from rust and should probably anyways be bumped with every release.
A minor downside of this is now that we have to different sources for the version. However, both versions are bumped by the same ci pipeline during
draft_new_release
- so eventually the source is the same.Simulator.Screen.Recording.-.iPhone.15.-.2024-03-06.at.16.19.14.mp4
fixes #2141
fixes #1748