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.

Sign up for GitHub

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

convert UpdateBalance to use relative amount instead of absolute amount change #219

Closed
wants to merge 1 commit into from
Closed

convert UpdateBalance to use relative amount instead of absolute amount change #219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions x/oracle/keeper/msg_server_create_price.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
package keeper

import (
"context"

Check failure on line 4 in x/oracle/keeper/msg_server_create_price.go

View workflow job for this annotation

GitHub Actions / Run golangci-lint 

File is not `gofumpt`-ed (gofumpt)
"errors"
"strconv"
"time"

Check failure on line 8 in x/oracle/keeper/msg_server_create_price.go

View workflow job for this annotation

GitHub Actions / Run golangci-lint 

File is not `gofumpt`-ed (gofumpt)
"crypto/sha256"

"github.com/ExocoreNetwork/exocore/x/oracle/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)

const (
layout = "2006-01-02 15:04:05"
maxFutureOffset = 5 * time.Second
layout = "2006-01-02 15:04:05"
maxFutureOffset = 5 * time.Second
maxEventPriceSize = 64
)

// CreatePrice proposes price for new round of specific tokenFeeder
Expand Down Expand Up @@ -63,10 +66,18 @@
decimalStr := strconv.FormatInt(int64(newItem.PriceTR.Decimal), 10)
tokenIDStr := strconv.FormatUint(newItem.TokenID, 10)
roundIDStr := strconv.FormatUint(newItem.PriceTR.RoundID, 10)
priceFormat := "price"
if len(newItem.PriceTR.Price) > maxEventPriceSize {
// This is mainly used for NST since they might have a string with big size to indicate the changes for stakers
hashPrice := sha256.Sum256([]byte(newItem.PriceTR.Price))
newItem.PriceTR.Price = string(hashPrice[:])
priceFormat = "hash"
}
Comment on lines +69 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Convert hash to a hexadecimal string for proper encoding.

Using string(hashPrice[:]) to convert the hash result into a string will produce unreadable characters because it interprets the byte array as a string of bytes. To correctly represent the hash as a readable string, it should be encoded in hexadecimal format.

Apply the following changes to fix the issue:

 priceFormat := "price"
 if len(newItem.PriceTR.Price) > maxEventPriceSize {
     // This is mainly used for NST since they might have a string with big size to indicate the changes for stakers
     hashPrice := sha256.Sum256([]byte(newItem.PriceTR.Price))
-    newItem.PriceTR.Price = string(hashPrice[:])
+    newItem.PriceTR.Price = fmt.Sprintf("%x", hashPrice)
     priceFormat = "hash"
 }

Additionally, ensure the fmt package is imported at the top of the file:

 import (
     "context"
     "errors"
     "strconv"
     "time"
+    "fmt"
     "crypto/sha256"
     "github.com/ExocoreNetwork/exocore/x/oracle/types"
     sdk "github.com/cosmos/cosmos-sdk/types"
 )
📝 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.

Suggested change
priceFormat := "price"
if len(newItem.PriceTR.Price) > maxEventPriceSize {
// This is mainly used for NST since they might have a string with big size to indicate the changes for stakers
hashPrice := sha256.Sum256([]byte(newItem.PriceTR.Price))
newItem.PriceTR.Price = string(hashPrice[:])
priceFormat = "hash"
}
priceFormat := "price"
if len(newItem.PriceTR.Price) > maxEventPriceSize {
// This is mainly used for NST since they might have a string with big size to indicate the changes for stakers
hashPrice := sha256.Sum256([]byte(newItem.PriceTR.Price))
newItem.PriceTR.Price = fmt.Sprintf("%x", hashPrice)
priceFormat = "hash"
}

ctx.EventManager().EmitEvent(sdk.NewEvent(
types.EventTypeCreatePrice,
sdk.NewAttribute(types.AttributeKeyRoundID, roundIDStr),
sdk.NewAttribute(types.AttributeKeyFinalPrice, tokenIDStr+"_"+roundIDStr+"_"+newItem.PriceTR.Price+"_"+decimalStr),
// [tokenIDStr]_[roundIDStr]_[price]_[decimal]_price/hash
sdk.NewAttribute(types.AttributeKeyFinalPrice, tokenIDStr+"_"+roundIDStr+"_"+newItem.PriceTR.Price+"_"+decimalStr+"_"+priceFormat),
sdk.NewAttribute(types.AttributeKeyPriceUpdated, types.AttributeValuePriceUpdatedSuccess)),
)
if !ctx.IsCheckTx() {
Expand Down
97 changes: 40 additions & 57 deletions x/oracle/keeper/native_token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,6 @@
"github.com/ethereum/go-ethereum/common/hexutil"
)

// deposit: update staker's totalDeposit
// withdoraw: update staker's totalDeposit
// delegate: update operator's price, operator's totalAmount, operator's totalShare, staker's share
// undelegate: update operator's price, operator's totalAmount, operator's totalShare, staker's share
// msg(refund or slash on beaconChain): update staker's price, operator's price

const (
NSTETHASSETID = "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee_0x65"
)

// SetStakerInfos set stakerInfos for the specific assetID
func (k Keeper) SetStakerInfos(ctx sdk.Context, assetID string, stakerInfos []*types.StakerInfo) {
store := ctx.KVStore(k.storeKey)
Expand All @@ -33,10 +23,6 @@
}
}

var maxEffectiveBalance = map[string]int{
NSTETHASSETID: 32,
}

// GetStakerInfo returns details about staker for native-restaking under asset of assetID
func (k Keeper) GetStakerInfo(ctx sdk.Context, assetID, stakerAddr string) types.StakerInfo {
store := ctx.KVStore(k.storeKey)
Expand Down Expand Up @@ -132,7 +118,16 @@
return ret
}

// UpdateValidatorListForStaker invoked when deposit/withdraw happedn for an NST asset
// deposit wiil increase the staker's balance with a new validatorPubkey added into that staker's validatorList
// withdraw will decrease the staker's balanec with a vadlidatorPubkey removed from that staker's validatorList
Comment on lines +121 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct typos in function comments

There are several typographical errors in the function comments. Correcting them will improve clarity.

Proposed changes:

-// UpdateValidatorListForStaker invoked when deposit/withdraw happedn for an NST asset
+// UpdateValidatorListForStaker invoked when deposit/withdraw happen for an NST asset
-// deposit wiil increase the staker's balance with a new validatorPubkey added into that staker's validatorList
+// Deposit will increase the staker's balance with a new validatorPubkey added into the staker's ValidatorPubkeyList
-// withdraw will decrease the staker's balanec with a vadlidatorPubkey removed from that staker's validatorList
+// Withdraw will decrease the staker's balance with a validatorPubkey removed from the staker's ValidatorPubkeyList
📝 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.

Suggested change
// UpdateValidatorListForStaker invoked when deposit/withdraw happedn for an NST asset
// deposit wiil increase the staker's balance with a new validatorPubkey added into that staker's validatorList
// withdraw will decrease the staker's balanec with a vadlidatorPubkey removed from that staker's validatorList
// UpdateValidatorListForStaker invoked when deposit/withdraw happen for an NST asset
// Deposit will increase the staker's balance with a new validatorPubkey added into the staker's ValidatorPubkeyList
// Withdraw will decrease the staker's balance with a validatorPubkey removed from the staker's ValidatorPubkeyList

func (k Keeper) UpdateNSTValidatorListForStaker(ctx sdk.Context, assetID, stakerAddr, validatorPubkey string, amount sdkmath.Int) error {
_, decimalInt, err := k.getDecimal(ctx, assetID)
if err != nil {
return err
}
// transfer amount into integer, for restaking the effective balance should always be whole unit
amountInt64 := amount.Quo(decimalInt).Int64()
Comment on lines +125 to +130
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure amount is a multiple of decimalInt to prevent rounding errors

When converting amount to whole units, any fractional part will be truncated. To avoid potential loss of value, consider adding a check to confirm that amount is exactly divisible by decimalInt.

Proposed code change:

+// Ensure that amount is a multiple of decimalInt
+if !amount.Mod(decimalInt).IsZero() {
+    return errors.New("amount must be a whole number of units")
+}
 amountInt64 := amount.Quo(decimalInt).Int64()
📝 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.

Suggested change
_, decimalInt, err := k.getDecimal(ctx, assetID)
if err != nil {
return err
}
// transfer amount into integer, for restaking the effective balance should always be whole unit
amountInt64 := amount.Quo(decimalInt).Int64()
_, decimalInt, err := k.getDecimal(ctx, assetID)
if err != nil {
return err
}
// transfer amount into integer, for restaking the effective balance should always be whole unit
// Ensure that amount is a multiple of decimalInt
if !amount.Mod(decimalInt).IsZero() {
return errors.New("amount must be a whole number of units")
}
amountInt64 := amount.Quo(decimalInt).Int64()

// emit an event to tell that a staker's validator list has changed
ctx.EventManager().EmitEvent(sdk.NewEvent(
types.EventTypeCreatePrice,
Expand All @@ -146,7 +141,7 @@
stakerInfo = types.NewStakerInfo(stakerAddr, validatorPubkey)
} else {
k.cdc.MustUnmarshal(value, stakerInfo)
if amount.IsPositive() {
if amountInt64 > 0 {
// deopsit add a new validator into staker's validatorList
stakerInfo.ValidatorPubkeyList = append(stakerInfo.ValidatorPubkeyList, validatorPubkey)
}
Comment on lines +144 to 147
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo and prevent adding duplicate validators

Correct the typo in the comment. Additionally, to prevent duplicate validator entries, add a check to ensure validatorPubkey is not already in ValidatorPubkeyList before appending.

Proposed changes:

-// deopsit add a new validator into staker's validatorList
+// Deposit adds a new validator into the staker's ValidatorPubkeyList
+// Check if validatorPubkey already exists
+exists := false
+for _, vPubkey := range stakerInfo.ValidatorPubkeyList {
+    if vPubkey == validatorPubkey {
+        // Validator already exists, do not add
+        exists = true
+        break
+    }
+}
+if !exists {
     stakerInfo.ValidatorPubkeyList = append(stakerInfo.ValidatorPubkeyList, validatorPubkey)
+}
📝 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.

Suggested change
if amountInt64 > 0 {
// deopsit add a new validator into staker's validatorList
stakerInfo.ValidatorPubkeyList = append(stakerInfo.ValidatorPubkeyList, validatorPubkey)
}
if amountInt64 > 0 {
// Deposit adds a new validator into the staker's ValidatorPubkeyList
// Check if validatorPubkey already exists
exists := false
for _, vPubkey := range stakerInfo.ValidatorPubkeyList {
if vPubkey == validatorPubkey {
// Validator already exists, do not add
exists = true
break
}
}
if !exists {
stakerInfo.ValidatorPubkeyList = append(stakerInfo.ValidatorPubkeyList, validatorPubkey)
}
}

Expand All @@ -159,7 +154,7 @@
newBalance.Index++
}
newBalance.Block = uint64(ctx.BlockHeight())
if amount.IsPositive() {
if amountInt64 > 0 {
newBalance.Change = types.Action_ACTION_DEPOSIT
} else {
// TODO: check if this validator has withdraw all its asset and then we can move it out from the staker's validatorList
Expand All @@ -174,19 +169,8 @@
}
}

decimal, decimalInt, err := k.getDecimal(ctx, assetID)
if err != nil {
return err
}

// the amount should be checked by caller
// in case of nstETH, deposit should be equal to 32e18 as the maxeffectivebalance
efbUnit := sdkmath.NewIntWithDecimal(int64(maxEffectiveBalance[assetID]), decimal)
if amount.GTE(efbUnit) {
newBalance.Balance += int64(maxEffectiveBalance[assetID])
} else {
newBalance.Balance += amount.Quo(decimalInt).Int64()
}
// TODO: should caller need extra check to make sure the amount is interger of unit
newBalance.Balance += amountInt64

keyStakerList := types.NativeTokenStakerListKey(assetID)
valueStakerList := store.Get(keyStakerList)
Expand All @@ -210,7 +194,7 @@
}
}
if !exists {
if !amount.IsPositive() {
if amountInt64 <= 0 {
return errors.New("remove unexist validator")
}
stakerList.StakerAddrs = append(stakerList.StakerAddrs, stakerAddr)
Expand Down Expand Up @@ -242,6 +226,8 @@
return nil
}

// TODO: currently we limit the change for a single staker no more than 16, this suites for beaconchain.
// may need to be upgraded to be compatible with other chains like solana
// UpdateNSTByBalanceChange updates balance info for staker under native-restaking asset of assetID when its balance changed by slash/refund on the source chain (beacon chain for eth)
func (k Keeper) UpdateNSTByBalanceChange(ctx sdk.Context, assetID string, rawData []byte, roundID uint64) error {
_, chainID, _ := assetstypes.ParseID(assetID)
Expand All @@ -257,59 +243,56 @@
return err
}
store := ctx.KVStore(k.storeKey)
for _, stakerAddr := range sl.StakerAddrs {
// if stakerAddr is not in stakerChanges, then the change would be set to 0 which is expected
change := stakerChanges[stakerAddr]
for stakerAddr, change := range stakerChanges {
key := types.NativeTokenStakerKey(assetID, stakerAddr)
value := store.Get(key)
if value == nil {
return errors.New("stakerInfo does not exist")
}
stakerInfo := &types.StakerInfo{}
k.cdc.MustUnmarshal(value, stakerInfo)
newBalance := types.BalanceInfo{}
if length := len(stakerInfo.BalanceList); length > 0 {
newBalance = *(stakerInfo.BalanceList[length-1])
length := len(stakerInfo.BalanceList)
// length should always be greater than 0 since the staker must deposit first, then we can update balance change
if length <= 0 {
return errors.New("UpdateBalane should not be executed on an empty balanceList")
}
newBalance = *(stakerInfo.BalanceList[length-1])
Comment on lines +255 to +260
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in error message

Correct the typo in the error message: "UpdateBalane" should be "UpdateBalance".

Proposed code change:

 if length <= 0 {
-    return errors.New("UpdateBalane should not be executed on an empty balanceList")
+    return errors.New("UpdateBalance should not be executed on an empty balanceList")
}
📝 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.

Suggested change
length := len(stakerInfo.BalanceList)
// length should always be greater than 0 since the staker must deposit first, then we can update balance change
if length <= 0 {
return errors.New("UpdateBalane should not be executed on an empty balanceList")
}
newBalance = *(stakerInfo.BalanceList[length-1])
length := len(stakerInfo.BalanceList)
// length should always be greater than 0 since the staker must deposit first, then we can update balance change
if length <= 0 {
return errors.New("UpdateBalance should not be executed on an empty balanceList")
}
newBalance = *(stakerInfo.BalanceList[length-1])

newBalance.Block = uint64(ctx.BlockHeight())
if newBalance.RoundID == roundID {
newBalance.Index++
} else {
newBalance.RoundID = roundID
newBalance.Index = 0
}
newBalance.Change = types.Action_ACTION_SLASH_REFUND
// balance update are based on initial/max effective balance: 32
maxBalance := maxEffectiveBalance[assetID] * (len(stakerInfo.ValidatorPubkeyList))
balance := maxBalance + change
// there's one case that this delta might be more than previous Balance
// staker's validatorlist: {v1, v2, v3, v5}
// in one same block: withdraw v2, v3, v5, balance of v2, v3, v5 all be slashed by -16
// => amount: 32*4->32(by withdraw), the validatorList of feeder will be updated on next block, so it will report the balance change of v5: -16 as in the staker's balance change, result to: 32*4->32-> 32-16*3 = -16
// we will just ingore this misbehavior introduced by synchronize-issue, and this will be correct in next block/round
if balance > maxBalance || balance <= 0 {
// balance should not be able to be reduced to 0 by balance change
return errors.New("effective balance should never exceeds 32 for one validator and should be positive")
newBalance.Balance += int64(change)
decimal, _, err := k.getDecimal(ctx, assetID)
if err != nil {
return err
}

if delta := int64(balance) - newBalance.Balance; delta != 0 {
decimal, _, err := k.getDecimal(ctx, assetID)
if err != nil {
return err
}
if err := k.delegationKeeper.UpdateNSTBalance(ctx, getStakerID(stakerAddr, chainID), assetID, sdkmath.NewIntWithDecimal(delta, decimal)); err != nil {
return err
}
newBalance.Balance = int64(balance)
if err = k.delegationKeeper.UpdateNSTBalance(ctx, getStakerID(stakerAddr, chainID), assetID, sdkmath.NewIntWithDecimal(int64(change), decimal)); err != nil {
return err
}
// newBalance.Balance += int64(change)

stakerInfo.Append(&newBalance)
bz := k.cdc.MustMarshal(stakerInfo)
store.Set(key, bz)
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
return nil
}

// TODO: set a persistent state to track this number
// GetNSTTotalIndex returns the count of how many time the NST balance of assetID has been changed including sources of deposit/withdraw, balanceChange
func (k Keeper) GetNSTTotalIndex(ctx sdk.Context, assetID string) int64 {
stakerInfos := k.GetStakerInfos(ctx, assetID)
totalIndex := int64(0)
for _, stakerInfo := range stakerInfos {
totalIndex += int64(len(stakerInfo.BalanceList))
}
return totalIndex
}
Comment on lines +285 to +294
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Implement persistent state to track NST total index

The TODO comment suggests that storing the total index in a persistent state may improve performance by avoiding iteration over all stakers.

Would you like assistance in implementing a persistent state to track this number?


func (k Keeper) getDecimal(ctx sdk.Context, assetID string) (int, sdkmath.Int, error) {
decimalMap, err := k.assetsKeeper.GetAssetsDecimal(ctx, map[string]interface{}{assetID: nil})
if err != nil {
Expand Down
10 changes: 5 additions & 5 deletions x/oracle/keeper/native_token_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,18 +138,18 @@ func (ks *KeeperSuite) TestNSTLifeCycleOneStaker() {
// - 4.1 check stakerInfo
stakerInfo = ks.App.OracleKeeper.GetStakerInfo(ks.Ctx, assetID, stakerStr)
ks.Equal(types.BalanceInfo{
Balance: 59,
Balance: 49,
Block: 1,
RoundID: 11,
Index: 0,
Change: types.Action_ACTION_SLASH_REFUND,
}, *stakerInfo.BalanceList[3])
// check stakerAssetInfo is updated correctly in assets module, this should be triggered in assets module by oracle module's UpdateNSTByBalanceChange
stakerAssetInfo, _ = ks.App.AssetsKeeper.GetStakerSpecifiedAssetInfo(ks.Ctx, stakerID, assetID)
amount59 := sdkmath.NewIntWithDecimal(59, 18)
amount49 := sdkmath.NewIntWithDecimal(49, 18)
ks.Equal(assetstypes.StakerAssetInfo{
TotalDepositAmount: amount59,
WithdrawableAmount: amount59,
TotalDepositAmount: amount49,
WithdrawableAmount: amount49,
PendingUndelegationAmount: sdk.ZeroInt(),
}, *stakerAssetInfo)

Expand All @@ -159,7 +159,7 @@ func (ks *KeeperSuite) TestNSTLifeCycleOneStaker() {
// - 5.1 check stakerInfo
stakerInfo = ks.App.OracleKeeper.GetStakerInfo(ks.Ctx, assetID, stakerStr)
ks.Equal(types.BalanceInfo{
Balance: 29,
Balance: 19,
Block: 1,
RoundID: 11,
Index: 1,
Expand Down
Loading