feat: Added in-memory storage for testing purposes#59
feat: Added in-memory storage for testing purposes#59Harshdev098 wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
Generally goes into the right direction, but we def. need to avoid re-allocating everything on every operation.
Harshdev098
force-pushed
the
memory_store
branch
2 times, most recently
from
4980a75 to
25d57e3
Compare
|
@tnull Have done the required changes |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Looks much better, but I think we still need to handle global_version properly, even if we're currently not using it client-side.
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
Harshdev098
force-pushed
the
memory_store
branch
from
25d57e3 to
9012e95
Compare
Harshdev098
force-pushed
the
memory_store
branch
from
9012e95 to
3b434d0
Compare
|
@tankyleo Can you please review it! |
|
🔔 3rd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tankyleo! This PR has been waiting for your review. |
rust/impls/src/in_memory_store.rs
Outdated
| version: record.version, | ||
| }), | ||
| }) | ||
| } else if request.key == GLOBAL_VERSION_KEY { |
There was a problem hiding this comment.
Looks like by the time we are here, we know the GLOBAL_VERSION_KEY does not have a value, otherwise guard.get would have returned Some previously. We can just return the GetObjectResponse below directly with version: 0.
There was a problem hiding this comment.
@Harshdev098 double checking things here, we still have this second branch the same as before ?
There was a problem hiding this comment.
Shouldn't we still return early if request.key == GLOBAL_VERSION_KEY rather than always first making the lookup?
There was a problem hiding this comment.
The first lookup handles cases where the GLOBAL_VERSION_KEY is some non-zero value. We want to check whether it's been set to some value in the map before returning the initial value in this branch.
Harshdev098
force-pushed
the
memory_store
branch
from
3b434d0 to
e0c31bb
Compare
|
@tankyleo Have done with the required changes! Can you please review it |
Harshdev098
force-pushed
the
memory_store
branch
from
8003119 to
5898609
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
When testing integration with LDK Node locally I found that the tests are currently failing. I now opened #62 to add LDK Node integration tests to our CI here. It would be great if that could land first, and we could also add a CI job for the in-memory store as part of this PR then, ensuring the implementation actually works as expected.
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
@Harshdev098 Please rebase now that #62 landed to make use of the new CI checks here. |
|
🔔 3rd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 13th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
@tankyleo Can you please tabke a look at it! |
|
🔔 14th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 15th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
This needs a rebase by now unfortunately, sorry! |
|
🔔 16th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 17th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 18th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 19th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 20th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 21st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 22nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
Harshdev098
force-pushed
the
memory_store
branch
3 times, most recently
from
f571b29 to
06eeb34
Compare
5 participants
Have added in_memory store for testing purpose.
We can edit config file to use specific store either postgresql or memory