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

Split Modules from main function #17

Merged
merged 4 commits into from
Oct 10, 2022
Merged

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Oct 4, 2022

1st commit: split rpc request creation from the now-async call. Integration Tested ✅

this PR splits scheduler and http as mods so that main is much more manageable

@DanGould DanGould marked this pull request as ready for review October 6, 2022 20:56
@DanGould
Copy link
Contributor Author

DanGould commented Oct 6, 2022

4403560 Passes (manual) smoke/integration test ✅

Each commit should be linted (cargo +nightly fmt) and have cargo clippy warnings fixed, but I won't block merge on that. This is a PR in the right direction for code health.

@DanGould DanGould changed the title Split Channel Scheduler from main function Split Modules from main function Oct 6, 2022
@DanGould
Copy link
Contributor Author

DanGould commented Oct 6, 2022

819ad8a integration tested ✅ please review for design

Copy link
Collaborator

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Concept ACK

Just some thoughts based on a preliminary read though. Can't wait to see this turn into something amazing.

Also some acknowledgement for my previous work (via git commit msg) would be appreciated 🙏

};
let body = req.into_body();
let bytes = hyper::body::to_bytes(body).await?;
dbg!(&bytes); // this is correct by my accounts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: dbg! usage doesn't need to be on a separate line.

#[derive(Clone)]
pub struct Scheduler {
lnd: LndClient,
pjs: Arc<Mutex<HashMap<Script, ScheduledPayJoin>>>, // payjoins mapped by owned `scriptPubKey`s
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 like the idea of ScheduledPayJoin. We should just have a list of channels we want to open (funding amount + node we want to open with). And when payjoin requests come, we check the amount, and determine if it is enough to open a channel (or multiple), by looking at our list of "scheduled channels".

Maybe this is out of scope of this PR (since this is just a refactor so the codebase becomes more maintainable), but I think this would be a decent change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you determine the amount to put in the bip21? Just allow it to be arbitrary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we would have 2 admin endpoints: add scheduled channel, add bip-21 request.

"Add bip-21 request" will add to a map that associates recipient (owned) scriptPubKey to "payment constraints" (i.e. minimum acceptable amount, maximum acceptable amount, etc.) so when we receive a payment, we know whether we have requested it, and whether the payment satisfies requested amount.

Now when we actually receive a payment request (via an incoming bip-078 original psbt), we check:

  1. Whether the psbt contains outputs that match our recipient scriptPubKeys. If one or more do, we need to ensure that the outputs that match satisfy the value constraints. If not, we need to reject the payment (I'm not sure how this is usually done). If we deem the outputs okay, continue to 2.
  2. We need to check if the original psbt actually creates a valid tx (because this is our fallback in case we fail payjoin).
  3. We sum up the values of "owned" outputs of the psbt. This is the max value we can work with when opening channels. We check how many scheduled channels we can open given this value.
  4. We initiate opens of these picked scheduled channels and create proposal psbt that we send back to sender.

let (owned_vout, pj) =
self.find_matching_payjoin(&original_psbt).ok_or(SchedulerError::NoMatchingPayJoin)?;

// FIXME does this need to go before find_matching_payjoin(...) ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 this can go later to reduce unnecessary processing.

@DanGould
Copy link
Contributor Author

Tested ✅ rebased with cargo +fmt in between and credit to Evan for developing the concepts from scratch to draft

@DanGould DanGould merged commit 29ff1ad into payjoin:master Oct 10, 2022
@DanGould DanGould deleted the split-scheduler branch October 10, 2022 22:57
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.

2 participants