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.

Sign up for GitHub

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

Jump to bottom

Config: OrderManager upgrade #1737

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Config: OrderManager upgrade #1737

wants to merge 11 commits into from

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Dec 4, 2024 •
edited
Loading

  • Replaces *bool with a boolean in orderManager.Enabled and orderManager.RespectOrderHistoryLimits
  • Replaces negative FuturesTrackingSeekDuration with positive
  • Removes runtime default FuturesTrackingSeekDuration and replaces with an error

Stacked Dependencies

Type of change

  • New feature (non-breaking change which adds functionality)

@gbjk gbjk added the nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged label Dec 4, 2024
@gbjk gbjk self-assigned this Dec 4, 2024
@gbjk gbjk changed the title Config: Move OrderManager to version management Config: OrderManager upgrade Dec 4, 2024
@gbjk gbjk added the review me This pull request is ready for review label Dec 4, 2024
@gbjk gbjk force-pushed the feature/config_ordermanager branch from 76a5f04 to 9b7b71e Compare December 9, 2024 08:14
gbjk added 11 commits December 11, 2024 09:10
@gbjk
Config: Move assetEnabled upgrade to Version management
4d32d83
@gbjk
Assets: Do not error on asset not enabled, or disabled
9253089 
This became more messy with Disabling something that's defaulted to
disabled.
Taking an idealogical stance against erroring that what you want to have
done is already done.
@gbjk
CurrencyManager: Set AssetEnabled when StorePairs(enabled)
95d9c4f
@gbjk
RPCServer: Fix tests expecting StoreAssetPairFormat to enable the asset
f67c6fe 
Also assertifies
@gbjk
Bitfinex: Fix tests for MarginFunding subs
7b5b7d4
@gbjk
GCTWrapper: Improve TestMain clarity
7caff83
@gbjk
Linter: Fixes for exchange.go
ce57ece
@gbjk
BTSE: Add futures to testconfig
cb6f6f0
@gbjk
Exchanges: Rename StoreAssetPairStore
b6105a2 
Previously we were calling it "Format", but accepting everything from
the PairStore.
We were also defaulting to turning the Asset on.

Now callers need to get their AssetEnabled set as they want it, so
there's no magic

This change also moves responsibility for error wrapping outside to the
caller.
@gbjk
Config: AssetEnabled upgrade should respect assetTypes
06558ca 
Previously we ignored the field and just turned on everything.
I think that was because we couldn't get at the old value.
In either case, we have the option to do better, and respect the
assetEnabled value
@gbjk
Config: Move OrderManager upgrade to Version management
7b8726d
@gbjk gbjk force-pushed the feature/config_ordermanager branch from 9b7b71e to 7b8726d Compare December 12, 2024 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shazbert shazbert Awaiting requested review from shazbert

@gloriousCode gloriousCode Awaiting requested review from gloriousCode

@thrasher- thrasher- Awaiting requested review from thrasher-

At least 3 approving reviews are required to merge this pull request.

Assignees

@gbjk gbjk

Labels
nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged review me This pull request is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@gbjk