Move LQT tournament work on top of main #5217
Merged
+13,044
−380
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closes #5012. Testing shouldn't be required at this stage. - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > This is consensus-breaking, because validators will disagree about, e.g. what people are voting on.
Closes #5013. Diverges slightly from the ADR draft in that it creates a nested proto sub-message specific to the LQT. My rationale here was that this is nicer than prefixing every relevant field with "liquidity_tournament", both in the proto definitions, but also in the Rust code, because that way we can have a default for that entire set of parameters, and then the code can explicitly mark the absence of that set as to be interpreted as this default. Also works nicer for the future, in which we might be using the funding component for other things, each of which can cleanly have its own set of parameters. I also added a little utility type for representing percentages. Not strictly necessary, but I think it's a good move towards type safety. The motivation here was all of the annoyance of dealing with keeping track of what u64s were bps or bps^2 or yadda yadda yadda in other parts of the codebase. I've added some relevant unit tests. - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > adds new parameters that can be voted on, so consensus-breaking. --------- Co-authored-by: Erwan Or <erwanor@penumbralabs.xyz>
- new commitment source kind to track the providence of LQT reward notes references #5011 - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:
## Describe your changes Implements the LQT rewards comprising the total reward pool size for an epoch. ## Issue ticket number and link references #5025 ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > adds new logic for computing LQT issuance
## Describe your changes This PR: - expose a component level api `LqtRead` - define two new DEX state key modules: `lqt::v1::lp` and `lqt::v1::pair` - implements a `position_manager::volume_tracker` - stubs out the inner position manager entrypoint, deferring implementation to later ## Volume definition We track the **outflow** of staking tokens from the position. This means that an attacker controlled asset must commit to a staking token inventory for at least a full block execution. ## State key modeling The lookup index maps an epoch index and a position id to a cumulative volume tally. The full sorted index orders position ids by cumulative volume (keyed to the epoch). ## Issue ticket number and link Part of #5015 ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. N/A - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch --------- Signed-off-by: Erwan Or <erwan.ounn.84@gmail.com> Co-authored-by: Lúcás Meier <lucas@cronokirby.com>
## Describe your changes Close #5028. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > Consensus and state breaking
Closes #5014. This implements the action for LQT voting, up to (and including) stateless checks. This is similar to delegator voting, but with some light simplification. This also reuses the strategy of UndelegateClaim in using an inner proof shared with a different circuit, which is, imo, cleaner code since the fact that the delegator vote circuit can be reused is a happy coincidence. Testing deferred. - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > you betcha --------- Signed-off-by: Lúcás Meier <cronokirby@gmail.com> Co-authored-by: Erwan Or <erwanor@penumbralabs.xyz>
## Describe your changes state key for epoch-scoped nullifier set (mapping nullifiers with their associated transaction id), serving as a precursor for performing the necessary stateful [nullifier check](https://github.com/penumbra-zone/penumbra/pull/5033/files#diff-a0986b8d223ab5b1c5536ba06bde1ede6d08f635eb97b386549ecfb55a4f2a4bR112). Additionally, augments the transaction context with `TransactionId`. ## Issue ticket number and link references #5029 ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch
Closes #5032. The check and execute logic for the action handler is still missing the nullifier check, but there's an obvious insertion point for adding that logic. Testing deferred.
## Describe your changes This trait is responsible for the actual accounting of moving funds around, to be later consumed by the end epoch handler, which figures out what portion of the rewards needs to go where. The bank will just actually move fractions of its budget in the community pool to the requested locations, atomically. Testing deferred. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > yes indeed
Right now this is only ever a transaction id, and for LQT work, we need to be able to just use a transaction ID, so it's better to reify that, and change a few uses to wrap it in a source, versus having to unwrap it, introducing a spurious failure case Smoke tests should be sufficient. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > This state is always initialized before being read, so there's no issue in changing the type we store under this state key ; reviewers should double check this.
## Describe your changes This uses the newly added ambient tx id to populate the LQT nullifier key, making the logic in check and execute final. This also removes the addition of the tx id to the TransactionContext. My understanding was that this was thought to be necessary to plumb the tx id for this action. Not particularly opposed to keeping that though. Testing deferred. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:
- Implements rpc services for the distributions component in `DistributionService` - indexes distributions state key by epoch index and writes to NV storage @cronokirby had some suggestions (which can be discussed in this PR) for replacing the historical epoch RPC, so I'm punting on the impl for now. references #5039 - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch
add LQT pool size increase event to the distributions component, emitted at the end of each block. references #5041 - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch
## Describe your changes This integrates the recent work in the funding component, providing a full implementation of the reward calculation and distribution logic, running at the end of each epoch. There are some unit and prop tests for some core tallying logic, but we'll need to do manual testing of the various RPCs an what not later. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:
## Describe your changes This leans much more heavily on the database for indexing and ranking, such that the only bit of in-memory storage is just the number of assets that can be considered at most, based on the threshold. Downside is that more of the logic exists as the sum of the behavior across state keys, and some tests were removed. Adding some tests at the component level here which read and write against temp storage would be useful ; perhaps mocking the implementation of reward distribution or something. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:
## Describe your changes This is probably the most important one to get right. Previously this was zero, but this commit changes it to be the same cost as an output action. This is because, in the worst case scenario, each vote results in minting a note. Fewer notes can be minted because we cap the number of voters each asset will tally, but in practice we might be well under this limit. No testing needed. ## Issue ticket number and link Closes #5053. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:
**Overview** In this PR, we: - Relax the inner guard to allow withdrawn LP updates without `seq+1` - Simplify the reward deposit logic so that it sticks to depositing rewards **Context** Before updating position states, the DEX engine runs an inner guard to defend against ostensibly invalid state transitions (e.g, Closed -> Open). In particular, since there was no mechanism to update an LP without also updating its sequence number, that particular transition was disallowed. This is no longer the case with the implementation of epoch-scoped LQTs. **Other validation** The guard is relaxed and removes a third layer of defense, the other two are: 1. The `withdraw_position` entrypoint called by the action handler 2. Ordinary check that the transaction value balances (burn, mint inc. seq) ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. This is tested once we test the end-to-end multi-withdraw mechanism. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT target
implement funding rpc that allows the client to track whether a nullifier has already been used in the current epoch’s voting period. This _isn't_ a bulk nullifier point query. references #5058 - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch
## Describe your changes This PR adds a component API to allow `funding` to deposit into a validator's delegation pool. It does so by exposing a `ValidatorPoolDeposit::deposit_to_validator_pool` method to safely handle it. One tension in the current design, is that the LQT logic lives in the `funding` component which execute last. As a result, delegation pool increases do not affect the validator's voting power until the following epoch. LQT rewards however, are immediately useful to awardees and can be directed towards future LQT seasons. One notable change to this PR is that we now track voters based on: - the beneficiary address for the rewards - the identity key of the validator corresponding to the delegation note used for voting **Alternative solution** If we want to solve this another way, we could close the current PR, and refactor the LQT logic into its own component. The approach should be the same: the `lqt` component calls the safe pool increase handle and the `staking` component takes care of the rest. ## Issue ticket number and link Discussing and overview in #5046 ## Checklist before requesting a review - [X] I have added guiding text to explain how a reviewer should test these changes. - [X] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT
defines and stubs `LqtVotingNotes` and `TournamentVotes` view service rpcs. - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch
appends `AddressIndex` field to `TournamentVotesRequest` references #5062 - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch
This PR adds events for the LQT tournament - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch
## Describe your changes lqt: fix epoch <=> start position comparison This allows planners to correctly use the first position in the epoch. This works because the ZK proof verifies that the start position is strictly less. c.f. https://github.com/penumbra-zone/penumbra/blob/7869803be0b844f09c8c5efeeaa92384b8a6ba35/crates/core/component/governance/src/delegator_vote/proof.rs#L234 Testing deferred. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:
Hello, I'm here from the future, and I'm rewriting this commit because we're rebasing the branch on top of a version in which we've already edited the app version to 10, because of an unplanned upgrade, and so we're changing this to be 11 now. Bumps the protocol version to denote the addition of LQT functionality. Also bumps the crate versions from 1.0.x -> 2.0.x, as an alpha. No tag has been created, to avoid triggering the formal release workflows, since we're still testing. I'm submitting this PR ahead of a planned testnet upgrade, so we'll shortly learn how effective the overall upgrade process is. However, special attention should be given to the proposed versions strings. In particular, the transition from `0.81.x` to `1.0.x` maintained compatibility with APP_VERSION 9, and that may present difficulties during the upgrade for nodes on one or the other version. - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > explicitly denotes consensus-breaking changes, and therefore targets a protocol feature branch, rather than main
We had an unplanned upgrade to version 10, so instead this becomes version 11 when rebasing Follow-up to #5073, which should have included a no-op migration. Towards #5010. This work will be used to perform a chain upgrade on the PL testnet. Once that's done, we should expect CI to pass fully on this PR, in particular the "testnet-integration" suite. - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > provides migration logic for proposed protocol-breaking changes
## Describe your changes Seems like the effect hash calculation for LQT votes was wrong, in that it erroneously included the authorization data and proof, which would make such actions effectively impossible to construct. This corrects things so that the effect hash is only calculated over the actual body of the vote. Testing deferred. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > This breaks consensus not only with main, but also with the tentatively applied testnet upgrade 😬
Previously we had a todo!() there, woops. I've actually tested that this, combined with changes in another staged pr, allow one to submit votes. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > Consensus breaking relative to testnet too.
Stacked pr on #5077, please review and merge that first. This implements all of the bits and bobs necessary to make `pcli tx lqt-vote` work. I ran a devnet and tested that you can submit votes. The events for voting do get emitted, but there seems to be some issue with either the code or the setup I had in terms of rewards actually being distributed. To test this pr, spinning up a devnet, or hitting a testnet post #5077 patch would work. - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > Not consensus breaking actually, relative to the protocol branch, or at least, it shouldn't be, since this only touches the client side of things. --------- Co-authored-by: Erwan Or <erwanor@penumbralabs.xyz>
Bumps the version in the LQT branch. Doing this to disambiguate between active versions, in order to roll out #5075 to the active testnet. Refs #5010, #5075. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > version info only, no code changes
This is a stop gap we probably want to revert, but can unblock testing.
This reverts commit fa70326.
pairs with penumbra-zone/web#2227 cc @conorsch - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch
`tournamentVotes` should be a streaming rpc simplification based on #5167 (comment) - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > LQT branch
## Describe your changes This fixes some issues with the calculation of the summary view for delegators. - The epochs voted in was calculated incorrectly. - Rewards were not being tallied correctly, leading to overstatement of them. - In case the user had a continuous streak of voting, without any gaps, the streak would be reported incorrectly. To test, it should be possible to simply run pindexer again, with no need to reset the database (being able to change views on the fly paying off!), and one should observe much more reasonable results with: ```sql select epochs_voted_in, total_rewards / 10^6, total_voting_power / 10 ^ 6, streak from lqt.delegator_summary; ``` ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > indexing only
## Describe your changes This implements a new binary, `elcuity`, which provides 3 different commands for voting, liquidity provision, and swapping, in an automated loop. These are all written with the intention of using a remote view service (e.g. pclientd) running in the background. I've added a README with usage information in this PR. I've tested the various commands, and they seem to work, although there are some discrepancies with the frontend so far. But, it is able to provide liquidity, repeatedly vote, swap, etc. For testing I'd recommend trying to follow the README and see if any unclear behaviors or oddities arise. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > REPLACE THIS TEXT WITH RATIONALE (CAN BE BRIEF) --------- Co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>
This adds a new, optional, parameter to the LQT knobs which adds an end block to the rewards. The way the mechanism is supposed to work is that once that block is reached, no further rewards will be distributed. This also adds an optimization in the base case where rewards are turned off, to not execute any of the LQT distribution logic at all. (If the amount of rewards to distribute is 0, you don't bother trying to fetch dex information and all that jazz). For testing easiest way is probably setting up a devnet, and testing that LQT is only enabled for a fixed time with this change. On testnet this should be able to be applied transparently, and we could even try a temporary shutoff and startup again with some governance votes. - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > This is consensus breaking, in that if you exercised a governance vote with the new params, you would observe a divergence in nodes pre and post this patch.
## Describe your changes This augments the `lqt.summary` view to have information about the start and end block (predicted or actual) along with an estimate of when the block will end, in seconds (matching the Figma design). This also changes the way rewards are presented in the summary to reflect the amount of rewards we *project* to be available during the epoch. ## Issue ticket number and link Closes #5178. I tested this by inspecting the data on testnet after these changes. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > indexing only
Uses 32 bit integers consistently, for the sake of JS land
This also avoids a division by 0 error if for some reason the only LPs in an epoch have 0 points.
We had multiple entries per epoch, when we should instead have been aggregating them.
I observed that for an unfinished epoch, the projected rewards were consistently about half of the final rewards. After some spelunking, I realized that this was expected, because only delegators have been receiving rewards, and they have only half of those allocated to them. There was indeed a bug, but only in pindexer, where we were not considering any lingering rewards carried over from the previous epoch.
## Describe your changes We accidentally omitted which we need for the LQT lp leaderboard. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > indexing only
) ## Describe your changes We were incorrectly selecting for the most recent non-zero gap, by not handling the most recent epoch a given delegator has voted in. In that case, there's no "end" to the gap, and we need to handle the value being NULL. The effect of this change is that streaks are now calculated correctly, with the weird behavior of seeing a streak larger than the number of epochs voted in not appearing. Closes penumbra-zone/web#2334 ## Issue ticket number and link ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > indexing only
## Describe your changes we were not handling LQT rewards, which increase available liquidity. Thus, when someone withdrew rewards, the total liquidity could become negative. This should fix penumbra-zone/web#2412. To test, we should run pindexer again and see that we don't run into the issue again. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > indexing only
## Describe your changes Implements the UIP for encrypted position metadata. Not currently any integration in terms of actually adding metadata to positions, and so far the implementation is very generous in accepting old versions of transaction plans and things of the sort, to not break any clients. For testing, I believe smoke tests should be sufficient, but we may also want to try running the effect hash generators to make sure we didn't accidentally break that. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > This is consensus-breaking. Maybe this should be directed towards protocol/lqt instead? --------- Co-authored-by: Erwan Or <erwanor@penumbralabs.xyz>
|
|
2 tasks
2 tasks
conorsch
added a commit
that referenced
this pull request
Jun 13, 2025
## Describe your changes view service definitions for #5212 base branch is currently `protocol/lqt_branch` and will be changed to main after #5217 lands, so it can be included in the next tagged release. This PR is a retargetting of #5218, which was merged into the wrong branch. ## Issue ticket number and link ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: --------- Co-authored-by: Tal Derei <talderei99@gmail.com> Co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PRIVACY IS A CULT
PENUMBRA IS THE KEY
UM IS THE TOKEN
(please do not squash merge this PR, for the love of all that is holy and sacred in this world)