Skip to content

Conversation

@yyforyongyu
Copy link
Member

@yyforyongyu yyforyongyu commented May 18, 2022

Saving the params to a file can be quite cumbersome when it comes to migration, and also is prone to bugs. This PR replaces #490 by saving the data into db instead.

Note that we are saving the RPC request here, which has a nice feature as it handles compatibility very well, as long as the tag numbers are used correctly.

TODO:

  • unit test

@yyforyongyu yyforyongyu requested review from arshbot and bhandras May 18, 2022 19:45
@yyforyongyu yyforyongyu force-pushed the db-params branch 3 times, most recently from 7cd7652 to 585db71 Compare May 20, 2022 04:55
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, nice work! 💯

@yyforyongyu yyforyongyu force-pushed the db-params branch 2 times, most recently from 075d5b3 to e566280 Compare May 23, 2022 01:18
@lightninglabs-deploy
Copy link

@arshbot: review reminder

Copy link
Contributor

@arshbot arshbot left a comment

Choose a reason for hiding this comment

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

These changes look good to me! Can we add documentation to the relevant command (perhaps right in the help of setparam) to inform users of how to clear saved params?

This commit refactors the `liquidity` by moving `Parameters` related
code into one file.
This commit refactors the method `manager.SetParameters` to take a
`SetLiquidityParamsRequest`. As we'll see in the following commit, this
will enable us saving the params to disk more easily.
This commit adds a new bucket to save liquidity parameters. We've
skipped the serialization and deserialization implementations here and
leave them to be handled by the liquidity package.
@yyforyongyu
Copy link
Member Author

These changes look good to me! Can we add documentation to the relevant command (perhaps right in the help of setparam) to inform users of how to clear saved params?

Updated the description, PTAL

@yyforyongyu yyforyongyu requested a review from arshbot June 7, 2022 11:42
This commit saves the RPC request used to construct the `Parameters` on
disk. Since it's a proto message, an easy way to read/write it is to
rely on the proto marshal/unmarshal methods. A side effect is that
migration also becomes easy as proto message have its own internal
mechanism to keep track of the compatibility.
Copy link
Contributor

@arshbot arshbot left a comment

Choose a reason for hiding this comment

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

LGTM! Great description too :D

@yyforyongyu yyforyongyu merged commit dfe50e4 into lightninglabs:master Jun 7, 2022
@yyforyongyu yyforyongyu deleted the db-params branch June 7, 2022 15:48
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