refactor(launchpad): remove unused currentHeight params and fix naming#1238
refactor(launchpad): remove unused currentHeight params and fix naming#1238
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
WalkthroughRemoved blockchain-height usage from launchpad deposits, reward collection, withdrawals, getters, and tests; flows now use Unix timestamps (currentTime) and deposit created/withdrawn heights and related getters were removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant LP as Launchpad
participant Runtime as Runtime
participant RM as RewardManager
participant Store as Store
Caller->>LP: CollectDepositGns(depositID)
LP->>Runtime: time.Now().Unix()
Runtime-->>LP: currentTime
LP->>RM: collectDepositReward(deposit, currentTime)
RM->>RM: updateRewardPerDepositX128(currentTime)
RM->>RM: collectReward(deposit, currentTime)
RM-->>LP: rewardAmount
alt rewardAmount > 0
LP->>Store: persist reward totals
Store-->>LP: ack
end
LP->>LP: withdrawDeposit(deposit, currentTime)
LP->>deposit: SetWithdrawn(currentTime)
LP->>Store: save updated deposit
Store-->>LP: ack
LP-->>Caller: returned withdrawal amount
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/r/gnoswap/launchpad/v1/launchpad_withdraw_test.gno (1)
617-622:⚠️ Potential issue | 🟡 MinorStale
currentHeightused for withdrawal - potential test inconsistency.The
currentHeightused at line 621 was captured at line 567 during project creation, buttesting.SkipHeights(tt.skipHeight)is called at line 608 before the withdrawal. This means the withdrawal receives a height that's significantly older than the actual chain state.This is inconsistent with
TestLaunchpadWithdraw_RewardStateRemoval(lines 512-514) wherecurrentHeightis correctly captured afterSkipHeights.Suggested fix
// Withdraw specific deposit withdrawDepositID := depositIDs[tt.withdrawDepositIndex] deposit, _ := getDeposit(withdrawDepositID) currentTime = time.Now().Unix() + currentHeight = runtime.ChainHeight() _, _, err := lp.withdrawDeposit(deposit, currentHeight, currentTime) uassert.NoError(t, err)
🧹 Nitpick comments (3)
contract/r/scenario/upgrade/implements/v3_valid/launchpad/launchpad_withdraw.gno (1)
63-127: UnusedcurrentHeightparameter - consider removal or consistent usage.The
currentHeightparameter passed tolp.withdrawDeposit(line 63) is never used within the function. At line 115,runtime.ChainHeight()is called directly instead of using the passedcurrentHeight:// Line 115: ignores the currentHeight parameter withdrawalAmount := withdrawDeposit(deposit, runtime.ChainHeight(), currentTime)This creates dead code since the caller captures
runtime.ChainHeight()at line 39 but it's ignored. Either:
- Remove
currentHeightfrom the signature (consistent with thecollectDepositRewardchange in this PR)- Use the passed
currentHeightparameter at line 115Given the PR's objective to remove unused height parameters, option 1 would be more consistent.
♻️ Suggested fix - use the passed parameter or remove it
Option A: Use the passed parameter
// Finalize withdrawal - withdrawalAmount := withdrawDeposit(deposit, runtime.ChainHeight(), currentTime) + withdrawalAmount := withdrawDeposit(deposit, currentHeight, currentTime)Option B: Remove the unused parameter (preferred for consistency with this PR)
-func (lp *launchpadV1) withdrawDeposit(deposit *launchpad.Deposit, currentHeight, currentTime int64) (address, int64, error) { +func (lp *launchpadV1) withdrawDeposit(deposit *launchpad.Deposit, currentTime int64) (address, int64, error) {contract/r/gnoswap/launchpad/v1/launchpad_withdraw.gno (1)
84-148: Inconsistent use ofcurrentHeightparameter.The
currentHeightparameter is passed tolp.withdrawDepositbut then ignored. At line 131,runtime.ChainHeight()is called again instead of using the passed value:// Line 84: currentHeight parameter received func (lp *launchpadV1) withdrawDeposit(deposit *launchpad.Deposit, currentHeight, currentTime int64) ... // Line 131: parameter ignored, runtime.ChainHeight() called instead withdrawalAmount := withdrawDeposit(deposit, runtime.ChainHeight(), currentTime)Per the context snippet from
deposit.gno, this height is recorded aswithdrawnHeighton the deposit. While the difference is negligible in practice (same block), this creates:
- Unnecessary duplicate
runtime.ChainHeight()call- The
currentHeightparameter becomes dead code within this functionThe PR description states this parameter is "retained because it forwards height to
Deposit.SetWithdrawn()", but it doesn't actually forward the caller-supplied value.♻️ Suggested fix - use the passed parameter
// Finalize withdrawal - withdrawalAmount := withdrawDeposit(deposit, runtime.ChainHeight(), currentTime) + withdrawalAmount := withdrawDeposit(deposit, currentHeight, currentTime)contract/r/gnoswap/launchpad/v1/deposit_test.gno (1)
384-388: Consider assertingWithdrawnHeight()to maintain test coverage.The hardcoded
123is a magic number, and the test no longer validates thatdeposit.WithdrawnHeight()is correctly set after withdrawal. Since theDepositstruct still trackswithdrawnHeightfor audit purposes (as shown indeposit.gno), consider adding an assertion to verify this field:amount := withdrawDeposit(deposit, 123, tt.currentTime) uassert.Equal(t, tt.expectedAmount, amount) uassert.Equal(t, tt.currentTime, deposit.WithdrawnTime()) + uassert.Equal(t, int64(123), deposit.WithdrawnHeight()) uassert.Equal(t, tt.expectedRemaining, deposit.DepositAmount()) uassert.Equal(t, tt.expectedWithdrawn, deposit.IsWithdrawn())Alternatively, define a named constant for the test height value to improve readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a56857a1-012a-4dae-abcd-4c2a36fc07d0
📒 Files selected for processing (10)
contract/r/gnoswap/launchpad/v1/deposit_test.gnocontract/r/gnoswap/launchpad/v1/launchpad_reward.gnocontract/r/gnoswap/launchpad/v1/launchpad_reward_test.gnocontract/r/gnoswap/launchpad/v1/launchpad_withdraw.gnocontract/r/gnoswap/launchpad/v1/launchpad_withdraw_test.gnocontract/r/gnoswap/launchpad/v1/project_tier_test.gnocontract/r/gnoswap/launchpad/v1/reward_manager_test.gnocontract/r/gnoswap/launchpad/v1/reward_state_test.gnocontract/r/scenario/upgrade/implements/v3_valid/launchpad/launchpad_reward.gnocontract/r/scenario/upgrade/implements/v3_valid/launchpad/launchpad_withdraw.gno
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contract/r/gnoswap/launchpad/deposit.gno (1)
105-111: Consider consolidating duplicate setters forwithdrawnTime.Both
SetWithdrawnTime()(line 81-83) andSetWithdrawn()(line 109-111) perform identical operations on the same field. This could lead to confusion about which method to use.♻️ Option 1: Delegate SetWithdrawn to SetWithdrawnTime
func (d *Deposit) SetWithdrawn(withdrawnTime int64) { - d.withdrawnTime = withdrawnTime + d.SetWithdrawnTime(withdrawnTime) }♻️ Option 2: Remove SetWithdrawn and use SetWithdrawnTime directly
If
SetWithdrawnwas primarily used to set both height and time (now simplified), consider removing it and having callers useSetWithdrawnTime()directly. This reduces API surface area.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f518df06-cf69-45da-b695-600a5fcc63bc
📒 Files selected for processing (20)
contract/r/gnoswap/launchpad/_mock_test.gnocontract/r/gnoswap/launchpad/counter_test.gnocontract/r/gnoswap/launchpad/deposit.gnocontract/r/gnoswap/launchpad/getter.gnocontract/r/gnoswap/launchpad/getter_test.gnocontract/r/gnoswap/launchpad/project_condition_test.gnocontract/r/gnoswap/launchpad/store_test.gnocontract/r/gnoswap/launchpad/types.gnocontract/r/gnoswap/launchpad/v1/assert_test.gnocontract/r/gnoswap/launchpad/v1/deposit.gnocontract/r/gnoswap/launchpad/v1/deposit_test.gnocontract/r/gnoswap/launchpad/v1/getter.gnocontract/r/gnoswap/launchpad/v1/getter_test.gnocontract/r/gnoswap/launchpad/v1/launchpad_deposit.gnocontract/r/gnoswap/launchpad/v1/launchpad_reward_test.gnocontract/r/gnoswap/launchpad/v1/project_tier_test.gnocontract/r/gnoswap/launchpad/v1/reward_manager_test.gnocontract/r/scenario/upgrade/implements/v3_valid/launchpad/deposit.gnocontract/r/scenario/upgrade/implements/v3_valid/launchpad/getter.gnocontract/r/scenario/upgrade/implements/v3_valid/launchpad/launchpad_deposit.gno
💤 Files with no reviewable changes (9)
- contract/r/gnoswap/launchpad/v1/assert_test.gno
- contract/r/scenario/upgrade/implements/v3_valid/launchpad/launchpad_deposit.gno
- contract/r/gnoswap/launchpad/v1/launchpad_deposit.gno
- contract/r/gnoswap/launchpad/getter.gno
- contract/r/gnoswap/launchpad/types.gno
- contract/r/gnoswap/launchpad/_mock_test.gno
- contract/r/scenario/upgrade/implements/v3_valid/launchpad/getter.gno
- contract/r/gnoswap/launchpad/v1/getter_test.gno
- contract/r/gnoswap/launchpad/v1/getter.gno
✅ Files skipped from review due to trivial changes (3)
- contract/r/gnoswap/launchpad/store_test.gno
- contract/r/gnoswap/launchpad/project_condition_test.gno
- contract/r/gnoswap/launchpad/v1/deposit_test.gno
🚧 Files skipped from review as they are similar to previous changes (4)
- contract/r/gnoswap/launchpad/v1/project_tier_test.gno
- contract/r/gnoswap/launchpad/v1/launchpad_reward_test.gno
- contract/r/gnoswap/launchpad/v1/deposit.gno
- contract/r/scenario/upgrade/implements/v3_valid/launchpad/deposit.gno
|
jinoosss
left a comment
jinoosss
left a comment
There was a problem hiding this comment.
We should consider whether removing the height information from the deposit is the right approach.
From the perspective of contract users, this data may be necessary.
Additionally, since launchpad deposits are reversible, we should consider implementing this change.
3 participants
Summary
Continue the cleanup started in #1236 by removing remaining unused
currentHeightparameters and fixing misleading naming in launchpad contract tests.Changes
Removed parameters (dead code)
collectDepositReward(deposit, currentHeight, currentTime)launchpad_reward.gnocurrentHeightwas accepted but never referenced in the function bodycurrentHeight) inUpdateRewardPerDepositX128,CollectReward,newRewardManagerreward_manager_test.gnott.currentHeightKept parameters (structurally required)
createProjectParams.currentHeightlaunchpad_project.gnoNewProject()which uses it forMakeProjectID(tokenPath, createdHeight)— project ID generation depends on itNewDeposit(..., createdHeight, ...)launchpad_deposit.gnoDeposit.createdHeightfor audit trackingwithdrawDeposit(d, currentHeight, currentTime)deposit.gnoDeposit.SetWithdrawn()which setswithdrawnHeight, used byIsWithdrawn()withdrawDeposit(deposit, currentHeight, currentTime)methodlaunchpad_withdraw.gnoruntime.ChainHeight()Propagation fix
runtime.ChainHeight()is now called at the top-level entry point (CollectDepositGns) and passed down as a parameter, instead of being called redundantly in nested functions. This makes the call chain consistent: entry points capture height/time, inner functions receive them as arguments.Renamed test fields (
currentHeight→currentTime)Several test struct fields were named
currentHeightbut their values were passed to time-based functions (isRewardStateClaimable,IsActivated,IsEnded), which acceptcurrentTime int64. Renamed for accuracy:reward_state_test.gno— field + test descriptionsproject_tier_test.gno— field nameSummary by CodeRabbit
Bug Fixes
Breaking Changes
Tests