-
Notifications
You must be signed in to change notification settings - Fork 129
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
sauron: Add mutinynet support #584
base: master
Are you sure you want to change the base?
Conversation
f343745
to
7519cc6
Compare
f80de9a
to
0e7c007
Compare
0e7c007
to
b8ca105
Compare
You need to pin the version of |
You might want to add some instructions in the README for how to start sauron with Mutinynet. Below command worked for me once I had |
a5483ee
to
70dc8ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments
@sip-21 Can you please give me more details about the tests you wrote and what were you trying to do when Sauron failed? I am trying to replicate it to fix it. Thanks! |
@ca-ruz The failing GitHub actions/tests are here: https://github.com/sip-21/plugins/actions/runs/11116007452/job/30885476517?pr=15
The tests run fine together locally, but in GitHub action
|
37fe65a
to
5091652
Compare
@sip-21 Thanks!! I recreated the bug and now it's fixed! Can you please run the tests again and let me know? |
bef972b
to
7402d7d
Compare
Waiting on @sip-21's PR here ca-ruz#2 (comment) |
007996c
to
ec3295c
Compare
Co-authored-by: sip21 <[email protected]> Co-authored-by: iorch <[email protected]>
ec3295c
to
ece41a9
Compare
.ci/test.py
Outdated
@@ -241,13 +241,14 @@ def run_one(p: Plugin, workflow: str) -> bool: | |||
|
|||
logging.info(f"Virtualenv at {vpath}") | |||
|
|||
num_workers = 1 if p.name == "sauron" else 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sip-21 didn't you fix the concurrency? We can get rid of this I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If removing this breaks the tests, we should add the changes to this file back, but under the other commit (the one that adds the tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these lines from the code. Tests are all failing now.
@@ -1,2 +1,2 @@ | |||
pyln-client>=23.2 | |||
pyln-client>=23.2,<=24.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create an issue to fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created Issue: #618.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're almost there! Just a couple of minor issues.
After adding back concurrency, the tests are failing because they're trying to listen on port 9735... that actually doesn't seem necessary. Can you fix the tests not to require listening on port 9735? (or maybe just listen on a random port) |
|
||
self.daemon = LightningD(lightning_dir, None) # noqa: F405 | ||
options = { | ||
"disable-plugin": "bcli", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to override this class at all. Just set the options appropriately when using the NodeFactory.get_node()
. That'll obviate the need for the subclass and the fixtures.
Please don't override the fixtures and the node class unless you absolutely need to. |
@cdecker do you have an example of any pytests that use the JSON schema from CLN to check the fields and types? That sounds amazing! |
It's done automatically by instrumenting the pyln-client library to load and check the schemas. Anything going through pyln-client in a test environment should be checked afaik. I don't think notifications and hooks are checked yet (there are no schemas yet), but that'd be an amazing extension covering the docs vs implementation too |
Adds mutinynet support to sauron. Mutinynet.com uses the mempool.space REST API, which has a slightly different spec from blockstream's Esplora so there is new code to handle that.
We pinned pyln-client to <=24.5 because version 24.8 has some extra requirements that sauron does not meet, we should probably fix this but for now pinning 24.5 should work fine.