fix(halt): allow emergency recovery settlement paths#1257
fix(halt): allow emergency recovery settlement paths#1257junghoon-vans wants to merge 11 commits intomainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRemoved many operation-specific halt guards, added a deferred pending-protocol-fee buffering/collection mechanism, introduced protocol-fee distributed-history APIs and getters, updated protocol_fee AddToProtocolFee to return an error, and adjusted tests to reflect emergency-mode recovery semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RouterV1
participant Store
participant ProtocolFee
participant Token
Client->>RouterV1: handleSwapFee(tokenPath, amount)
RouterV1->>RouterV1: tryCollectPendingProtocolFees()
RouterV1->>Store: GetPendingProtocolFees()
alt pending exists and not halted
RouterV1->>Token: SafeGRC20Approve(protocolFeeAddr, amount)
RouterV1->>ProtocolFee: AddToProtocolFee(tokenPath, amount)
ProtocolFee-->>RouterV1: (ok / error)
RouterV1->>Store: SetPendingProtocolFees(updatedMap)
else protocol fee halted or no pending
RouterV1->>Store: addPendingProtocolFee(tokenPath, amount) (SetPendingProtocolFees)
end
RouterV1->>Client: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
How about removing halt from gnft?
It seems better for nft to function as a neutral resource rather than being included in the domain.
Can you share your thoughts? @dongwon8247
jinoosss
left a comment
jinoosss
left a comment
There was a problem hiding this comment.
I agree that the domain and withdrawal functions should be completely separated.
However, if the protocol fee, which is called internally by the contract, is halted, it should not affect other functions.
For example, Router Swap, Collect Fee / Reward, etc.
In my opinion, we need to consider how to handle the halt of the AddToProtocolFee function in ProtocolFee.
When the protocol fee is halted, we may need to manage any pending amounts that have not yet been processed.
| // Note: Default split is 0% devOps, 100% gov/staker. | ||
| func (pf *protocolFeeV1) DistributeProtocolFee() map[string]int64 { | ||
| halt.AssertIsNotHaltedProtocolFee() | ||
| halt.AssertIsNotHaltedProtocolFeeOrWithdraw() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contract/r/gnoswap/gov/xgns/xgns_test.gno (1)
391-403:⚠️ Potential issue | 🟡 MinorReset the withdraw halt bit before asserting burn recovery.
Burnnow gates onOpTypeWithdraw, but this setup only resets/enablesOpTypeXGns. Please clearOpTypeWithdrawas part of this test setup so the “Burn still allowed” case is isolated from any prior halt state.🧪 Proposed test isolation fix
func resetHalt(t *testing.T) { t.Helper() if adminAddr, ok := access.GetAddress(prbac.ROLE_ADMIN.String()); ok { testing.SetRealm(testing.NewUserRealm(adminAddr)) halt.SetOperationStatus(cross, halt.OpTypeXGns, false) + halt.SetOperationStatus(cross, halt.OpTypeWithdraw, false) } }contract/r/gnoswap/gov/staker/v1/staker_reward.gno (1)
166-175:⚠️ Potential issue | 🟡 MinorUpdate the halt-condition docs for the split gate.
Line 168 still documents withdrawal halt as the only halt condition, but Lines 171-175 now block additions on
gov_stakerand removals onwithdraw.Proposed doc fix
// Panics: // - if caller is not the launchpad contract -// - if system is halted for withdrawals +// - if add is true and gov_staker operations are halted +// - if add is false and withdrawals are halted // - if access control operations fail func (gs *govStakerV1) SetAmountByProjectWallet(addr address, amount int64, add bool) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4259040-e9ac-4b54-afde-154d7c980fa7
⛔ Files ignored due to path filters (15)
contract/r/scenario/halt/emergency_mode_gov_staker_settlement_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/emergency_mode_governance_propose_execute_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/emergency_mode_launchpad_growth_blocked_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/emergency_mode_launchpad_settlement_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/emergency_mode_position_recovery_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/emergency_mode_protocol_fee_settlement_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/emergency_mode_staker_recovery_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_gov_staker_settlement_blocked_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_governance_and_withdraw_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_governance_propose_execute_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_launchpad_growth_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_launchpad_settlement_blocked_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_position_recovery_blocked_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_protocol_fee_settlement_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_staker_recovery_blocked_filetest.gnois excluded by!**/*filetest.gno
📒 Files selected for processing (31)
contract/r/gnoswap/community_pool/community_pool.gnocontract/r/gnoswap/community_pool/community_pool_test.gnocontract/r/gnoswap/emission/emission.gnocontract/r/gnoswap/gnft/_helper_test.gnocontract/r/gnoswap/gnft/gnft.gnocontract/r/gnoswap/gnft/gnft_test.gnocontract/r/gnoswap/gov/governance/v1/governance_propose.gnocontract/r/gnoswap/gov/staker/v1/staker_delegate.gnocontract/r/gnoswap/gov/staker/v1/staker_delegate_test.gnocontract/r/gnoswap/gov/staker/v1/staker_reward.gnocontract/r/gnoswap/gov/staker/v1/staker_reward_test.gnocontract/r/gnoswap/gov/xgns/xgns.gnocontract/r/gnoswap/gov/xgns/xgns_test.gnocontract/r/gnoswap/halt/assert.gnocontract/r/gnoswap/halt/assert_test.gnocontract/r/gnoswap/halt/config_test.gnocontract/r/gnoswap/halt/getters_test.gnocontract/r/gnoswap/launchpad/v1/launchpad_protocol_fee.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/pool/v1/pool.gnocontract/r/gnoswap/pool/v1/protocol_fee.gnocontract/r/gnoswap/position/v1/position.gnocontract/r/gnoswap/position/v1/position_test.gnocontract/r/gnoswap/position/v1/reposition.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_test.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/staker.gnocontract/r/gnoswap/staker/v1/staker_test.gno
💤 Files with no reviewable changes (11)
- contract/r/gnoswap/gov/governance/v1/governance_propose.gno
- contract/r/gnoswap/gov/staker/v1/staker_delegate.gno
- contract/r/gnoswap/pool/v1/protocol_fee.gno
- contract/r/gnoswap/position/v1/reposition.gno
- contract/r/gnoswap/emission/emission.gno
- contract/r/gnoswap/launchpad/v1/launchpad_withdraw.gno
- contract/r/gnoswap/staker/v1/staker.gno
- contract/r/gnoswap/launchpad/v1/launchpad_reward.gno
- contract/r/gnoswap/community_pool/community_pool.gno
- contract/r/gnoswap/launchpad/v1/launchpad_protocol_fee.gno
- contract/r/gnoswap/staker/v1/external_incentive.gno
junghoon-vans
force-pushed
the
GSW-2521-emergency-recovery-halt
branch
from
7ab5163 to
b6266fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
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/protocol_fee/v1/protocol_fee_test.gno (1)
1245-1265:⚠️ Potential issue | 🟡 MinorAssert halted accrual leaves token balances unchanged.
This test verifies the accounting map stays zero, but it would still pass if
AddToProtocolFeetransferred tokens and then returned the halt error before recording them. Capture holder/protocol-fee balances before the call and assert both are unchanged.Proposed test strengthening
holderRealm := testing.NewCodeRealm(poolPath) fundAndApproveProtocolFee(holderRealm, "gno.land/r/onbloc/bar", 100) testing.SetRealm(holderRealm) + holderBefore := common.BalanceOf("gno.land/r/onbloc/bar", holderRealm.Address()) + protocolFeeBefore := common.BalanceOf("gno.land/r/onbloc/bar", protocolFeeAddr) // When - Then if tt.shouldPanic { @@ uassert.NotPanics(t, func() { func(cur realm) { err := pf.AddToProtocolFee("gno.land/r/onbloc/bar", 100) uassert.NotNil(t, err) }(cross) }) + uassert.Equal(t, holderBefore, common.BalanceOf("gno.land/r/onbloc/bar", holderRealm.Address())) + uassert.Equal(t, protocolFeeBefore, common.BalanceOf("gno.land/r/onbloc/bar", protocolFeeAddr)) if tt.assertState != nil { tt.assertState(t, pf) }
🧹 Nitpick comments (1)
contract/r/gnoswap/pool/v1/init.gno (1)
69-74: Naming inconsistency across modules.
HasPendingProtocolFees()here differs fromHasPendingProtocolFeesKey()(router) andHasPendingProtocolFeesStoreKey()(staker). Not a bug, but consider aligning the naming for consistency since these three modules implement the same pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b75d3b4d-bc31-4148-acf3-03ff8c25a75c
⛔ Files ignored due to path filters (9)
contract/r/scenario/gov/staker/complex_protocol_fee_reward_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/gov/staker/launchpad_protocol_fee_reward_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/gov/staker/launchpad_protocol_fee_reward_with_blocked_withdraw_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/gov/staker/staker_protocol_fee_reward_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_launchpad_growth_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_protocol_fee_settlement_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/protocol_fee/balance_amount_mismatch_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/protocol_fee/distribute_protocol_fee_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/protocol_fee/distribute_with_different_devops_pct_filetest.gnois excluded by!**/*filetest.gno
📒 Files selected for processing (29)
contract/r/gnoswap/gov/staker/v1/state.gnocontract/r/gnoswap/mock/pool_store.gnocontract/r/gnoswap/mock/router_store.gnocontract/r/gnoswap/pool/store.gnocontract/r/gnoswap/pool/types.gnocontract/r/gnoswap/pool/v1/init.gnocontract/r/gnoswap/pool/v1/manager.gnocontract/r/gnoswap/pool/v1/protocol_fee.gnocontract/r/gnoswap/protocol_fee/_mock_test.gnocontract/r/gnoswap/protocol_fee/proxy.gnocontract/r/gnoswap/protocol_fee/types.gnocontract/r/gnoswap/protocol_fee/v1/_mock_test.gnocontract/r/gnoswap/protocol_fee/v1/errors.gnocontract/r/gnoswap/protocol_fee/v1/getter.gnocontract/r/gnoswap/protocol_fee/v1/init.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_test.gnocontract/r/gnoswap/router/store.gnocontract/r/gnoswap/router/types.gnocontract/r/gnoswap/router/v1/_mock_test.gnocontract/r/gnoswap/router/v1/init.gnocontract/r/gnoswap/router/v1/protocol_fee_swap.gnocontract/r/gnoswap/staker/store.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/_mock_test.gnocontract/r/gnoswap/staker/v1/init.gnocontract/r/gnoswap/staker/v1/protocol_fee_unstaking.gnocontract/r/gnoswap/staker/v1/protocol_fee_unstaking_test.gno
✅ Files skipped from review due to trivial changes (1)
- contract/r/gnoswap/protocol_fee/v1/errors.gno
🚧 Files skipped from review as they are similar to previous changes (3)
- contract/r/gnoswap/protocol_fee/proxy.gno
- contract/r/gnoswap/protocol_fee/v1/protocol_fee.gno
- contract/r/gnoswap/protocol_fee/types.gno
| func (pfs *protocolFeeState) distributeToDevOps(token string, amount int64) error { | ||
| if err := pfs.addAccuToDevOps(token, amount); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| devOpsAddr := access.MustGetAddress(prabc.ROLE_DEVOPS.String()) | ||
| common.SafeGRC20Transfer(cross, token, devOpsAddr, amount) | ||
|
|
||
| if err := pfs.addActualDistributedToDevOps(token, amount); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // distributeToGovStaker distributes tokens to Gov/Staker and updates related state. | ||
| // Amount should be greater than 0 (already checked in DistributeProtocolFee). | ||
| func (pfs *protocolFeeState) distributeToGovStaker(token string, amount int64) error { | ||
| if err := pfs.addAccuToGovStaker(token, amount); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| govStakerAddr := access.MustGetAddress(prabc.ROLE_GOV_STAKER.String()) | ||
| common.SafeGRC20Transfer(cross, token, govStakerAddr, amount) | ||
|
|
||
| if err := pfs.addActualDistributedToGovStaker(token, amount); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Move actual-distribution state updates before token transfers.
Lines 67 and 80 perform external token transfers before Lines 69 and 82 persist the new actual-distributed counters. If the store update returns an error after a successful transfer, the token movement can be completed without corresponding accounting. Update history first; a transfer panic should roll the transaction back.
As per coding guidelines, all state updates must occur before token transfers (Checks-Effects-Interactions pattern).
Proposed fix
func (pfs *protocolFeeState) distributeToDevOps(token string, amount int64) error {
devOpsAddr := access.MustGetAddress(prabc.ROLE_DEVOPS.String())
- common.SafeGRC20Transfer(cross, token, devOpsAddr, amount)
if err := pfs.addActualDistributedToDevOps(token, amount); err != nil {
return err
}
+ common.SafeGRC20Transfer(cross, token, devOpsAddr, amount)
+
return nil
}
@@
func (pfs *protocolFeeState) distributeToGovStaker(token string, amount int64) error {
govStakerAddr := access.MustGetAddress(prabc.ROLE_GOV_STAKER.String())
- common.SafeGRC20Transfer(cross, token, govStakerAddr, amount)
if err := pfs.addActualDistributedToGovStaker(token, amount); err != nil {
return err
}
+ common.SafeGRC20Transfer(cross, token, govStakerAddr, amount)
+
return nil
}| for tokenPath, amount := range pendingProtocolFees { | ||
| if amount <= 0 { | ||
| delete(pendingProtocolFees, tokenPath) | ||
| continue | ||
| } | ||
|
|
||
| common.SafeGRC20Approve(cross, tokenPath, protocolFeeAddr, amount) | ||
| if err := pf.AddToProtocolFee(cross, tokenPath, amount); err != nil { | ||
| return | ||
| } | ||
| delete(pendingProtocolFees, tokenPath) | ||
| } | ||
|
|
||
| if err := s.store.SetPendingProtocolFees(pendingProtocolFees); err != nil { |
There was a problem hiding this comment.
Persist each successful pending-fee collection before a later error can discard it.
If one pending token is collected successfully and a later pf.AddToProtocolFee returns an error, Line 79 returns before Line 84 persists the cloned map. The already-collected token remains pending in storage and can be collected again on the next retry.
🐛 Proposed fix: persist progress per entry and restore only the failed entry
for tokenPath, amount := range pendingProtocolFees {
if amount <= 0 {
delete(pendingProtocolFees, tokenPath)
continue
}
+ delete(pendingProtocolFees, tokenPath)
+ if err := s.store.SetPendingProtocolFees(pendingProtocolFees); err != nil {
+ panic(err)
+ }
+
common.SafeGRC20Approve(cross, tokenPath, protocolFeeAddr, amount)
if err := pf.AddToProtocolFee(cross, tokenPath, amount); err != nil {
+ pendingProtocolFees[tokenPath] = safeAddInt64(pendingProtocolFees[tokenPath], amount)
+ if setErr := s.store.SetPendingProtocolFees(pendingProtocolFees); setErr != nil {
+ panic(setErr)
+ }
return
}
- delete(pendingProtocolFees, tokenPath)
}
junghoon-vans
force-pushed
the
GSW-2521-emergency-recovery-halt
branch
from
5e47984 to
0775780
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
contract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gno (1)
65-84:⚠️ Potential issue | 🟠 MajorMove actual-distribution accounting before token transfers.
SafeGRC20Transferexecutes beforeaddActualDistributedTo*; if the later store update returns an error, tokens can move without matching distributed-history accounting. Keep effects before interactions. As per coding guidelines, all state updates must occur before token transfers (Checks-Effects-Interactions pattern).Proposed fix
func (pfs *protocolFeeState) distributeToDevOps(token string, amount int64) error { devOpsAddr := access.MustGetAddress(prabc.ROLE_DEVOPS.String()) - common.SafeGRC20Transfer(cross, token, devOpsAddr, amount) if err := pfs.addActualDistributedToDevOps(token, amount); err != nil { return err } + common.SafeGRC20Transfer(cross, token, devOpsAddr, amount) + return nil } @@ func (pfs *protocolFeeState) distributeToGovStaker(token string, amount int64) error { govStakerAddr := access.MustGetAddress(prabc.ROLE_GOV_STAKER.String()) - common.SafeGRC20Transfer(cross, token, govStakerAddr, amount) if err := pfs.addActualDistributedToGovStaker(token, amount); err != nil { return err } + common.SafeGRC20Transfer(cross, token, govStakerAddr, amount) + return nil }contract/r/gnoswap/staker/v1/protocol_fee_unstaking.gno (1)
64-87:⚠️ Potential issue | 🔴 CriticalPersist partial collection progress before returning on an error.
If one token is collected and deleted from the cloned map, then a later
AddToProtocolFeereturns an error, the earlyreturnskipsSetPendingProtocolFees; the already-collected token remains pending in storage and can be collected again. Based on learnings, every fee transfer must be tracked correctly throughAddToProtocolFee.Proposed fix
common.SafeGRC20Approve(cross, tokenPath, protocolFeeAddr, amount) if err := pf.AddToProtocolFee(cross, tokenPath, amount); err != nil { + if setErr := s.store.SetPendingProtocolFees(pendingProtocolFees); setErr != nil { + panic(setErr) + } return } delete(pendingProtocolFees, tokenPath) }contract/r/gnoswap/pool/v1/protocol_fee.gno (1)
80-103:⚠️ Potential issue | 🔴 CriticalPersist partial collection progress before returning on
AddToProtocolFeeerrors.A successful earlier entry is deleted only from the cloned map. If a later token returns an error, Line 95 exits before Line 100 persists those deletions, so the already-collected fees can be retried and double-collected. Based on learnings, every fee transfer must be tracked correctly through
AddToProtocolFee.Proposed fix
common.SafeGRC20Approve(cross, tokenPath, protocolFeeAddr, amount) if err := pf.AddToProtocolFee(cross, tokenPath, amount); err != nil { + if setErr := i.store.SetPendingProtocolFees(pendingProtocolFees); setErr != nil { + panic(setErr) + } return } delete(pendingProtocolFees, tokenPath) }contract/r/gnoswap/router/v1/protocol_fee_swap.gno (1)
123-136:⚠️ Potential issue | 🔴 CriticalPersist per-token collection progress before external calls.
If a later
pf.AddToProtocolFeereturns an error at Line 130, earlier successful deletes only exist in the local clone and Line 136 is skipped, so already-collected fees can be retried and collected again. Persist the deletion before approval/collection, and restore the failed token only whenAddToProtocolFeereturns an error. As per coding guidelines, all state updates must occur before token transfers (Checks-Effects-Interactions pattern).🐛 Proposed fix
for tokenPath, amount := range pendingProtocolFees { if amount <= 0 { delete(pendingProtocolFees, tokenPath) continue } + delete(pendingProtocolFees, tokenPath) + if err := r.store.SetPendingProtocolFees(pendingProtocolFees); err != nil { + panic(err) + } + common.SafeGRC20Approve(cross, tokenPath, protocolFeeAddr, amount) if err := pf.AddToProtocolFee(cross, tokenPath, amount); err != nil { + pendingProtocolFees[tokenPath] = safeAddInt64(pendingProtocolFees[tokenPath], amount) + if setErr := r.store.SetPendingProtocolFees(pendingProtocolFees); setErr != nil { + panic(setErr) + } return } - delete(pendingProtocolFees, tokenPath) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34907148-de06-43ea-ad37-244fb7531dcb
⛔ Files ignored due to path filters (17)
contract/r/scenario/gov/staker/complex_protocol_fee_reward_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/gov/staker/launchpad_protocol_fee_reward_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/gov/staker/launchpad_protocol_fee_reward_with_blocked_withdraw_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/gov/staker/staker_protocol_fee_reward_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/emergency_mode_gov_staker_settlement_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/emergency_mode_launchpad_settlement_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/emergency_mode_protocol_fee_settlement_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_gov_staker_settlement_blocked_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_launchpad_growth_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_launchpad_settlement_blocked_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/halt/safe_mode_protocol_fee_settlement_allowed_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/launchpad/launchpad_deposit_project_multi_recipient_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/launchpad/launchpad_deposit_project_single_recipient_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/launchpad/launchpad_single_deposit_protocol_fee_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/protocol_fee/balance_amount_mismatch_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/protocol_fee/distribute_protocol_fee_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/protocol_fee/distribute_with_different_devops_pct_filetest.gnois excluded by!**/*filetest.gno
📒 Files selected for processing (35)
contract/r/gnoswap/gov/staker/v1/state.gnocontract/r/gnoswap/mock/pool_store.gnocontract/r/gnoswap/mock/router_store.gnocontract/r/gnoswap/pool/store.gnocontract/r/gnoswap/pool/types.gnocontract/r/gnoswap/pool/v1/init.gnocontract/r/gnoswap/pool/v1/manager.gnocontract/r/gnoswap/pool/v1/protocol_fee.gnocontract/r/gnoswap/protocol_fee/_mock_test.gnocontract/r/gnoswap/protocol_fee/proxy.gnocontract/r/gnoswap/protocol_fee/types.gnocontract/r/gnoswap/protocol_fee/v1/_mock_test.gnocontract/r/gnoswap/protocol_fee/v1/errors.gnocontract/r/gnoswap/protocol_fee/v1/getter.gnocontract/r/gnoswap/protocol_fee/v1/init.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_test.gnocontract/r/gnoswap/router/store.gnocontract/r/gnoswap/router/types.gnocontract/r/gnoswap/router/v1/_mock_test.gnocontract/r/gnoswap/router/v1/init.gnocontract/r/gnoswap/router/v1/protocol_fee_swap.gnocontract/r/gnoswap/staker/store.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/_mock_test.gnocontract/r/gnoswap/staker/v1/init.gnocontract/r/gnoswap/staker/v1/protocol_fee_unstaking.gnocontract/r/gnoswap/staker/v1/protocol_fee_unstaking_test.gnocontract/r/gnoswap/test/fuzz/_mock_test.gnocontract/r/scenario/upgrade/implements/mock/protocol_fee/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/protocol_fee/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/protocol_fee/test_impl.gnotests/integration/testdata/gov/governance/create_parameter_change_proposal.txtartests/integration/testdata/gov/governance/execute_text_proposal_should_fail.txtar
💤 Files with no reviewable changes (1)
- tests/integration/testdata/gov/governance/create_parameter_change_proposal.txtar
✅ Files skipped from review due to trivial changes (2)
- contract/r/gnoswap/gov/staker/v1/state.gno
- contract/r/gnoswap/protocol_fee/v1/errors.gno
🚧 Files skipped from review as they are similar to previous changes (18)
- contract/r/gnoswap/pool/v1/init.gno
- contract/r/gnoswap/protocol_fee/v1/init.gno
- contract/r/gnoswap/staker/v1/protocol_fee_unstaking_test.gno
- contract/r/gnoswap/router/types.gno
- contract/r/gnoswap/pool/types.gno
- contract/r/gnoswap/staker/store.gno
- contract/r/gnoswap/staker/types.gno
- contract/r/gnoswap/router/v1/_mock_test.gno
- contract/r/gnoswap/pool/store.gno
- contract/r/gnoswap/mock/pool_store.gno
- contract/r/gnoswap/protocol_fee/v1/getter.gno
- contract/r/gnoswap/mock/router_store.gno
- contract/r/gnoswap/router/v1/init.gno
- contract/r/gnoswap/staker/v1/_mock_test.gno
- contract/r/scenario/upgrade/implements/v2_invalid/protocol_fee/test_impl.gno
- contract/r/gnoswap/protocol_fee/proxy.gno
- contract/r/gnoswap/protocol_fee/types.gno
- contract/r/scenario/upgrade/implements/mock/protocol_fee/test_impl.gno
| poolAddr := access.MustGetAddress(prabc.ROLE_POOL.String()) | ||
| gns.TransferFrom(cross, previousRealmAddr, poolAddr, poolCreationFee) | ||
| i.addPendingProtocolFee(GNS_PATH, poolCreationFee) | ||
| i.tryCollectPendingProtocolFees() |
There was a problem hiding this comment.
Use the safe transfer wrapper and record pending fees before the token call.
gns.TransferFrom bypasses the project’s safe transfer wrapper, and the pending-fee state is written after the external token transfer. Use SafeGRC20TransferFrom and keep effects before interactions. As per coding guidelines, use SafeGRC20Transfer or SafeGRC20TransferFrom only for token transfers, and all state updates must occur before token transfers.
Proposed fix
- gns.TransferFrom(cross, previousRealmAddr, poolAddr, poolCreationFee)
i.addPendingProtocolFee(GNS_PATH, poolCreationFee)
+ common.SafeGRC20TransferFrom(cross, GNS_PATH, previousRealmAddr, poolAddr, poolCreationFee)
i.tryCollectPendingProtocolFees()If this removes the last gns usage, also drop the gno.land/r/gnoswap/gns import.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| poolAddr := access.MustGetAddress(prabc.ROLE_POOL.String()) | |
| gns.TransferFrom(cross, previousRealmAddr, poolAddr, poolCreationFee) | |
| i.addPendingProtocolFee(GNS_PATH, poolCreationFee) | |
| i.tryCollectPendingProtocolFees() | |
| poolAddr := access.MustGetAddress(prabc.ROLE_POOL.String()) | |
| i.addPendingProtocolFee(GNS_PATH, poolCreationFee) | |
| common.SafeGRC20TransferFrom(cross, GNS_PATH, previousRealmAddr, poolAddr, poolCreationFee) | |
| i.tryCollectPendingProtocolFees() |
| if err := pf.bookCollectedProtocolFees(); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| sentToDevOpsForEvent := make([]string, 0) | ||
| sentToGovStakerForEvent := make([]string, 0) | ||
| toReturnDistributedToGovStaker := make(map[string]int64) | ||
| if halt.IsHaltedWithdraw() { | ||
| return toReturnDistributedToGovStaker | ||
| } |
There was a problem hiding this comment.
Emit an event when safe-mode booking mutates accounting.
bookCollectedProtocolFees() can move tokenListWithAmounts into accumulated state and clear the token list, but Line 42 returns before any event when withdraw is halted. Either move the halt check before booking if this path should be a true no-op, or emit a dedicated booking/deferred-settlement event so indexers can observe the state change. As per coding guidelines, all state changes MUST emit events for off-chain indexing.
| // Check for overflow | ||
| addedAmount := safeAddInt64(currentAmount, amount) | ||
| if amount == 0 { | ||
| pf.store.SetTokenListWithAmountItem(tokenPath, addedAmount) | ||
| return nil | ||
| } | ||
| protocolFeeAddr := access.MustGetAddress(prabc.ROLE_PROTOCOL_FEE.String()) | ||
| common.SafeGRC20TransferFrom(cross, tokenPath, caller, protocolFeeAddr, amount) | ||
|
|
||
| pf.store.SetTokenListWithAmountItem(tokenPath, addedAmount) |
There was a problem hiding this comment.
Persist accounting before transferring tokens.
Line 214 performs an external token transfer before Line 216 records the new protocol-fee amount. Move the store update before SafeGRC20TransferFrom; a transfer panic should still roll back the transaction, while reentrant/external execution cannot observe stale accounting. As per coding guidelines, all state updates must occur before token transfers (Checks-Effects-Interactions pattern).
🛡️ Proposed fix
// Check for overflow
addedAmount := safeAddInt64(currentAmount, amount)
+ pf.store.SetTokenListWithAmountItem(tokenPath, addedAmount)
if amount == 0 {
- pf.store.SetTokenListWithAmountItem(tokenPath, addedAmount)
return nil
}
protocolFeeAddr := access.MustGetAddress(prabc.ROLE_PROTOCOL_FEE.String())
common.SafeGRC20TransferFrom(cross, tokenPath, caller, protocolFeeAddr, amount)
-
- pf.store.SetTokenListWithAmountItem(tokenPath, addedAmount)
return nil
junghoon-vans
force-pushed
the
GSW-2521-emergency-recovery-halt
branch
from
0775780 to
7c26d25
Compare
junghoon-vans
force-pushed
the
GSW-2616-protocol-fee-pull
branch
from
84dced3 to
48efe03
Compare
|
2 participants
Summary
OrWithdrawpanic messages so failures now explain that both the primary operation and withdraw fallback are blockedVerification
Summary by CodeRabbit
New Features
Tests
Breaking Changes