-
Notifications
You must be signed in to change notification settings - Fork 249
Feat/mvds00 run coinbase more refactoring #1993
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
base: devnet-ready
Are you sure you want to change the base?
Feat/mvds00 run coinbase more refactoring #1993
Conversation
…reparation of refactor
…e TotalStake and TotalIssuance once
| // --- 4. Injection. | ||
| // Actually perform the injection of alpha_in, alpha_out and tao_in into the subnet pool. | ||
| // This operation changes the pool liquidity each block. | ||
| for netuid_i in subnets_to_emit_to.iter() { |
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.
The reason this was made a separate loop was that injection back when we had uniswap v2 could change price, and this would disturb the balance between subnet emissions. We needed to calculate all emissions first using that prices that do not change (because emissions are proportional to prices), and then inject.
My concern here is that we are going to have a tokenomics fix that may or may not return to this behavior (i.e. change price on injection). It is safe to merge now, but will require more work for that update in the next step.
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.
Isn't this a YAGNI case? It was five minutes to join them, it will be five to split them, and only if needed. The certain benefit of having the simplest code (for all people involved) far outweighs the possible cost (for one developer) of a possible future change.
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.
FYI we want to merge this stuff but not this week, we have a bunch of stuff touching these areas coming in within the next 5 days so please let's revisit next week 🙏🏻
Description
Some more refactoring, not changing functionality (except some debug logging that is now gone).
Type of Change
Checklist
cargo fmtandcargo clippyto ensure my code is formatted and linted correctlyAdditional Notes
This PR is under debate in discord channel refactor-run_coinbase