Skip to content

Conversation

@sputn1ck
Copy link
Member

This PR modifies how we store liquidity params by adding an asset id key

// Unmarshal the params.
req := &clientrpc.LiquidityParameters{}
err = proto.Unmarshal(paramsBytes, req)
if len(params) != 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can move this above to the other param check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think emptiness check should still come first (just as it is now).

@sputn1ck sputn1ck force-pushed the autoloop_assetid_migration branch 2 times, most recently from de33223 to 5747a8e Compare January 29, 2025 10:14
Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the nicely sized PR, I've done a first pass.


// Put the liquidity parameters in the toStore.
err = m.toStore.PutLiquidityParams(ctx, params)
err = m.toStore.PutLiquidityParams(ctx, "btc", params[0].Params)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use constant defaultBtcAssetId.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this override future assedIDs other than defaultBtcAssetId?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, but this shouldn't be a problem I'd assum. New users will not run into this functions, and I'd assume most old users will have migrated already.

INSERT INTO liquidity_params (id, params)
SELECT 1, params
FROM liquidity_params_assets_backup
WHERE asset_id = 'BTC';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this "btc"?


-- Copy data from the old table to the new table
INSERT INTO new_liquidity_params (asset_id, params)
SELECT 'BTC', params FROM liquidity_params;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this 'btc'?

@sputn1ck sputn1ck force-pushed the autoloop_assetid_migration branch from 5747a8e to a180f2a Compare January 29, 2025 11:37
@sputn1ck sputn1ck self-assigned this Jan 29, 2025
@bhandras bhandras self-requested a review January 31, 2025 14:05
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 💯

// Unmarshal the params.
req := &clientrpc.LiquidityParameters{}
err = proto.Unmarshal(paramsBytes, req)
if len(params) != 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think emptiness check should still come first (just as it is now).

@@ -1,5 +1,5 @@
//go:build test_db_postgres
// +build test_db_postgres
//go:build !test_db_postgres
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tag guards were correct?

@lightninglabs-deploy
Copy link

@sputn1ck, remember to re-request review from reviewers when ready

@sputn1ck sputn1ck closed this Feb 18, 2025
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.

4 participants