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

Datastore #4674

Merged
merged 8 commits into from
Aug 25, 2021
Merged

Datastore #4674

merged 8 commits into from
Aug 25, 2021

Conversation

rustyrussell
Copy link
Contributor

Closes #4525

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM, I have only a few comments


l = l1.rpc.listdatastore()
# Order is undefined!
assert (l == {'datastore': [{'key': 'somekey', 'hex': somedata},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the place where the code check will fail.

I a little bit difficult to guess what is the correct format of the {} from the error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it's always better to be explicit in tests, than loose. And it works :)

wallet/wallet.c Outdated
Comment on lines 4712 to 4763
struct db_stmt *stmt;

/* Test if already exists. */
stmt = db_prepare_v2(w->db, SQL("SELECT 1"
" FROM datastore"
" WHERE key = ?;"));
db_bind_text(stmt, 0, key);
db_query_prepared(stmt);

if (db_step(stmt)) {
tal_free(stmt);
return false;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if I'm missing some things in the design of this functionality, but if a plugin needs to override functionality is it now possible with the current API of the wallet.h, right?

Do you think that is necessary use some additional parameters to the RPC command to give the possibility to override the propriety with key?

Somethings like lightning-cli datastore "fooo" "some" true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since this isn't going to be in this release, I can revise this properly.

I am going to allow both hex and string arguments, and also allow a mode argument:

  • "must-create", "must-replace", or "create-or-replace", "must-append", "create-or-append".

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this idea Rusty.

@niftynei niftynei added this to the v0.10.2 milestone Jul 22, 2021
rustyrussell added a commit to rustyrussell/plugins that referenced this pull request Jul 23, 2021
See ElementsProject/lightning#4674
(I manually tested that it passes all the tests there, too!)

When they finally upgrade the node, this automatically puts any
datastore data into the inbuilt datastore and shuts down.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Contributor Author

See also lightningd/plugins#275 which provides this same functionality for previous versions!

rustyrussell added a commit to rustyrussell/plugins that referenced this pull request Jul 24, 2021
See ElementsProject/lightning#4674
(I manually tested that it passes all the tests there, too!)

When they finally upgrade the node, this automatically puts any
datastore data into the inbuilt datastore and shuts down.

Signed-off-by: Rusty Russell <[email protected]>
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

rustyrussell added a commit to lightningd/plugins that referenced this pull request Jul 25, 2021
See ElementsProject/lightning#4674
(I manually tested that it passes all the tests there, too!)

When they finally upgrade the node, this automatically puts any
datastore data into the inbuilt datastore and shuts down.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Contributor Author

Updated to add generation field, on suggestion from @shesek !

@rustyrussell
Copy link
Contributor Author

rustyrussell commented Aug 17, 2021

Trivial rebase to fix flake.
x2

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: JSON-RPC: `datastore`, `deldatastore` and `listdatastore` for plugins to store simple persistent key/value data.
It's common, and the simplest case.

Signed-off-by: Rusty Russell <[email protected]>
We add a generation counter, and allow update or del conditional
on a given generation.

Formalizes error codes, too, since we have more now.

Suggested-by: @shesek
Signed-off-by: Rusty Russell <[email protected]>
After some discussion with @shesek, and my own usage, we agreed that
a more comprehensive interface, which explicitly supports grouping,
is desirable.

Thus keys are now arrays, with the semantic that a key is either a
parent or has a value, never both.

For convenience in the JSON schema, we always return them as arrays,
though we accept simple strings as arguments.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell rustyrussell force-pushed the guilt/datastore branch 2 times, most recently from c586d49 to 22da787 Compare August 25, 2021 02:52
@rustyrussell
Copy link
Contributor Author

rustyrussell commented Aug 25, 2021

ACK 22da787

@cdecker cdecker merged commit fe86c11 into ElementsProject:master Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Brainstorming: Storing plugin metadata in the database
4 participants