Skip to content
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

refactor(staker): staker #437

Closed
wants to merge 24 commits into from
Closed

refactor(staker): staker #437

wants to merge 24 commits into from

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Dec 12, 2024

Description

  • Remove unused internal functions and imports
  • Moved external incentive related functions from the staker file to a separate staker_external_incentive file
  • Extract condition check as functions
  • Change the parameters of the getTokenPairBalanceFromPosition function
    • The pool path is already obtained before this function is called with pn.PositionGetPositionPoolKey(tokenId) method, it is unnecessary to process it internally.
- func getTokenPairBalanceFromPosition(tokenId uint64) (string, string) {
+ func getTokenPairBalanceFromPosition(poolPath string, tokenId uint64) (string, string) {
- 	poolKey := pn.PositionGetPositionPoolKey(tokenId)
        // ...
}

Newly Added Test Files

  • manage_pool_tiers
  • reward_internal_emission
  • reward_manager
  • reward_recipient_store
  • staker

Test Result

Screenshot 2024-12-16 at 7 32 14 PM

@notJoon notJoon changed the title refactor: Staker refactor(staker): remove unused and split staker Dec 13, 2024
@notJoon notJoon requested review from onlyhyde and r3v4s December 13, 2024 02:58
@notJoon notJoon marked this pull request as ready for review December 13, 2024 02:58
@notJoon notJoon marked this pull request as draft December 13, 2024 10:41
@notJoon notJoon changed the title refactor(staker): remove unused and split staker refactor(staker): staker Dec 13, 2024
@notJoon notJoon marked this pull request as ready for review December 16, 2024 10:33
Copy link
Member

@r3v4s r3v4s left a comment

Choose a reason for hiding this comment

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

  • do not inline get -> data check -> if nil panic -> if not nil type assertion for avl data
    • make it as separate function
  • for testcase screenshot, run it from gnoswap's repo(with current br branch), not from gno

@notJoon
Copy link
Member Author

notJoon commented Dec 17, 2024

* for testcase screenshot, run it from gnoswap's repo(with current br branch), not from gno

I don't think this will be a problem, since I just moved the entire main branch over to the gno side.

@@ -518,3 +517,13 @@ func externalDepositGnsAmount() uint64 {

return amount
}


func getPoolTiers() map[string]InternalTier {
Copy link
Member

Choose a reason for hiding this comment

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

should be avl

Copy link
Member Author

@notJoon notJoon Dec 26, 2024

Choose a reason for hiding this comment

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

calc_pool_position_XXX prefix series files will be removed. don't need to change

if callerIsOwner { // if caller is owner, transfer NFT ownership to staker contract
transferDeposit(tokenId, consts.STAKER_ADDR)
if err := applyStake(result); err != nil {
panic(err)
Copy link
Member

Choose a reason for hiding this comment

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

no need err codes?

Copy link
Member Author

@notJoon notJoon Dec 26, 2024

Choose a reason for hiding this comment

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

The applyStake already returns errNoPermission in the transferDeposit function, so it does not have to handle it again.

// applyStake performs the actual staking operation by updating contract state.
func applyStake(s *stakeResult) error {
	deposits.Set(s.tokenId, s.deposit)

	if s.owner == s.caller {
		if err := transferDeposit(s.tokenId, s.owner, s.caller, consts.STAKER_ADDR); err != nil { // <-
			return err
		}
	}

	pn.SetPositionOperator(s.tokenId, s.caller)
	positionsInternalWarmUpAmount[s.tokenId] = warmUpAmount{}

	return nil
}

func transferDeposit(tokenId uint64, owner, caller, to std.Address) error {
	if caller == to {
		return ufmt.Errorf(
			"%v: only owner(%s) can transfer tokenId(%d), called from %s",
			errNoPermission, owner, tokenId, caller,
		) // <-
	}

	// transfer NFT ownership
	gnft.TransferFrom(a2u(owner), a2u(to), tid(tokenId))

	return nil
}

}

externalWarmUpAmount, exist := positionsExternalWarmUpAmount[tokenId][incentiveId]
externalWarmUpAmount, exist := positionsExternalWarmUpAmount[tokenId][ictvId]
Copy link
Member

Choose a reason for hiding this comment

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

Separate getter(and setter) will be much better for nested avl datas.

Copy link
Member Author

Choose a reason for hiding this comment

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

same as here: #437 (comment)

that global variable will be removed

// Add position's liquidity to total liquidity
positionLiquidity := position.GetLiquidity()
if positionLiquidity != nil {
p.totalLiquidity.Add(p.totalLiquidity, positionLiquidity)
Copy link
Member

Choose a reason for hiding this comment

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

liquidity is already huge number (that's why we use uint256 for it).
adding u256 + u256 does need overflow check.

Copy link
Member Author

Choose a reason for hiding this comment

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

@notJoon
Copy link
Member Author

notJoon commented Dec 31, 2024

branch has merged with PR #438

@notJoon notJoon closed this Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants