[MICRO]: lntypes: Add Dual[A] primitive type#8996
[MICRO]: lntypes: Add Dual[A] primitive type#8996Roasbeef merged 1 commit intolightningnetwork:masterfrom
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
I think those commits are better suited in the PR they are in as they are primarily motivated by the consolidation of the This specific commit actually did not originate in #8270 but instead from #8755 (94ccf88). I carved it off later because I realized it can be used in the #8270 work as well. All this is to say, I think it is properly scoped as is. |
While I'm wary of getting off the refactor tractor and never getting off, I think that pre-factor PRs are well justified when:
Without them this PR just introduces dead code, and doesn't really shift any review burden away from subsequent PRs. I think it's totally okay for a prep-work PR to exist in the context of its followup, especially when it's a nice cleanup like this. |
|
I don't mind this, but I've also been specific guidance by other members of the team at other times that they are less likely to review code that "doesn't seem to do anything". Packing stuff into Duals on its own doesn't appear useful to people and its utility is only realized when we start to index into them in the core systems. If I can get another member of the team to co-sign your request to include those commits in a pre-factor PR, I'll do it. What I'm wary of is making changes to get your sign off that causes me to be at direct odds with the requests of other members of the team. @yyforyongyu @guggero what do you think? If you're on board with what @carlaKC is asking, then I'm happy to make this the plan. I don't have a preference, I just want y'all to agree with each other so we can ship stuff plz. |
guggero
left a comment
There was a problem hiding this comment.
LGTM, just two nits and a naming suggestion. But non-blocking.
This commit introduces a new type Dual[A] to make it easier to manage symmetric configurations or state for lightning channels.
I think this PR is fine on its own. To me the goal/context is clear, since the PR that's going to use is already up. |
carlaKC
left a comment
There was a problem hiding this comment.
Still feel like we should slim down the dependent PR by adding the switchover to use Dual where we need it, but lgtm!
This commit introduces a new type Dual[A] to make it easier to manage symmetric configurations or state for lightning channels.
Change Description
I'm extracting this commit from #8755 because it is proving to be useful in #8270 as well. It has applications anywhere we have symmetric structure, which in the channel state machine is ubiquitous.
Steps to Test
N/A
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.