Skip to content

Conversation

@0xLaz3r
Copy link

@0xLaz3r 0xLaz3r commented Oct 28, 2025

Description

Contribution Checklist

  • PR title starts with (PE-<TICKET_NUMBER>)
  • Code approved
  • Tests approved
  • CI Tests pass

Checklist

  • Every contract variable/method declared as public/external private/internal
  • Consider if this PR needs the officeHours modifier override
  • Verify expiration (30 days unless otherwise specified)
  • Verify hash in the description matches here
  • Validate all addresses used are in changelog or known
  • Notify any external teams affected by the spell so they have the opportunity to review
  • Deploy spell ETH_GAS_LIMIT="XXX" ETH_GAS_PRICE="YYY" make deploy
  • Verify mainnet contract on etherscan
  • Change test to use mainnet spell address and deploy timestamp
  • Run make archive-spell or make date="YYYY-MM-DD" archive-spell to make an archive directory and copy DssSpell.sol, DssSpell.t.sol, DssSpell.t.base.sol, and DssSpellCollateralOnboarding.sol
  • squash and merge this PR

@coderabbitai
Copy link

coderabbitai bot commented Oct 28, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 2025-10-30

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

src/DssSpell.sol Outdated
/// @param usr The USDS receiver.
/// @param wad The USDS amount in wad precision (10 ** 18).
function _transferUsds(address usr, uint256 wad) internal {
// Note: Enforce whole units to avoid rounding errors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Note: Enforce whole units to avoid rounding errors.

Comment on lines +24 to +28
import { FlapperInit, KickerConfig } from "src/dependencies/dss-flappers/FlapperInit.sol";
// Note: code matches https://github.com/sky-ecosystem/star-guard/blob/52239d716a89188b303f137fc43fb9288735ba2e/deploy/StarGuardInit.sol
import { StarGuardInit, StarGuardConfig } from "src/dependencies/starguard/StarGuardInit.sol";
// Note: code matches https://github.com/sky-ecosystem/endgame-toolkit/blob/fe734bea271e87c0b8e772d7adcccb46c4df1939/script/dependencies/treasury-funded-farms/TreasuryFundedFarmingInit.sol
import { TreasuryFundedFarmingInit, FarmingInitParams } from "src/dependencies/lockstake/TreasuryFundedFarmingInit.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The reviewer checklist does mention single imports only (although I think this is cleaner).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this rule is applicable as checklist only mentions "Interfaces imported from dss-interfaces"
https://github.com/sky-ecosystem/pe-checklists/blob/393726a9adc7c7418fc7180482dfec8edc6b66fb/spell/spell-reviewer-mainnet-checklist.md?plain=1#L50

src/DssSpell.sol Outdated
StarGuardJobLike(STARGUARD_JOB).add(SPARK_STARGUARD);

// Add StarGuardJob to the Chainlog as CRON_STARGUARD_JOB
DssExecLib.setChangelogAddress("CRON_STARGUARD_JOB", STARGUARD_JOB);
Copy link
Member

Choose a reason for hiding this comment

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

Minor:
STARGUARD_JOB could be CRON_STARGUARD_JOB

Copy link
Member

Choose a reason for hiding this comment

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

Addressed already

src/DssSpell.sol Outdated
address internal constant REWARDS_LSSKY_SKY = 0xB44C2Fb4181D7Cb06bdFf34A46FdFe4a259B40Fc;
address internal constant REWARDS_DIST_LSSKY_SKY = 0x675671A8756dDb69F7254AFB030865388Ef699Ee;
address internal constant SPARK_STARGUARD = 0x6605aa120fe8b656482903E7757BaBF56947E45E;
address internal constant CRON_STARGUARD_JOB = 0xB18d211fA69422a9A848B790C5B4a3957F7Aa44E;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
address internal constant CRON_STARGUARD_JOB = 0xB18d211fA69422a9A848B790C5B4a3957F7Aa44E;
address internal constant CRON_STARGUARD_JOB = 0xB18d211fA69422a9A848B790C5B4a3957F7Aa44E;

Copy link
Member

Choose a reason for hiding this comment

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

Align the new var

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this file to its corresponding sub-directory, otherwise a diff is produced with the reviewed commit:

diff script/dependencies/treasury-funded-farms/TreasuryFundedFarmingInit.sol ../maker-mainnet-spells/src/dependencies/lockstake/TreasuryFundedFarmingInit.sol
18,19c18,19
< import {StakingRewardsInit, StakingRewardsInitParams} from "../StakingRewardsInit.sol";
< import {VestInit, VestCreateParams} from "../VestInit.sol";
---
> import {StakingRewardsInit, StakingRewardsInitParams} from "./StakingRewardsInit.sol";
> import {VestInit, VestCreateParams} from "./VestInit.sol";
22c22
< } from "../VestedRewardsDistributionInit.sol";
---
> } from "./VestedRewardsDistributionInit.sol";

The proposed structure would include an additional subfolder treasury-funded-farms inside lockstake with this file

Also nit: please prefer naming the directories in accordance to their repository names (endgame-toolkit, star-guard)


// Sanity checks
assertEq(rewards.rewardsDistribution(), address(dist), "testLsskySkyRewards/rewards-rewards-dist-mismatch");
assertEq(rewards.stakingToken(), lssky, "testLsskySkyRewards/rewards-staking-token-mismatch");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should revert with the error messages.

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.

6 participants