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

#42: refactor types and utils #86

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 14 additions & 2 deletions chainio/clients/avs_registry_contracts_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
regcoord "github.com/Layr-Labs/eigensdk-go/contracts/bindings/BLSRegistryCoordinatorWithIndices"
"github.com/Layr-Labs/eigensdk-go/logging"
"github.com/Layr-Labs/eigensdk-go/types"
"math/big"

"github.com/ethereum/go-ethereum/accounts/abi/bind"
gethcommon "github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -159,7 +160,7 @@ func (a *AvsRegistryContractsChainClient) GetOperatorQuorumsAtCurrentBlock(
if err != nil {
return nil, err
}
quorums := types.BitmapToQuorumIds(quorumBitmap)
quorums := bitmapToQuorumIds(quorumBitmap)
return quorums, nil
}

Expand All @@ -177,7 +178,7 @@ func (a *AvsRegistryContractsChainClient) GetOperatorsStakeInQuorumsOfOperatorAt
if err != nil {
return nil, nil, err
}
quorums := types.BitmapToQuorumIds(quorumBitmap)
quorums := bitmapToQuorumIds(quorumBitmap)
return quorums, blsOperatorStateRetrieverOperator, nil
}

Expand Down Expand Up @@ -217,3 +218,14 @@ func (a *AvsRegistryContractsChainClient) DeregisterOperator(
quorumNumbers,
pubkey)
}

func bitmapToQuorumIds(bitmap *big.Int) []types.QuorumNum {
// loop through each index in the bitmap to construct the array
quorumIds := make([]types.QuorumNum, 0, types.MaxNumberOfQuorums)
for i := 0; i < types.MaxNumberOfQuorums; i++ {
if bitmap.Bit(i) == 1 {
quorumIds = append(quorumIds, types.QuorumNum(i))
}
}
return quorumIds
}
34 changes: 34 additions & 0 deletions types/common.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package types

import (
"errors"
"net/url"
"regexp"
)

func isValidEthereumAddress(address string) bool {
re := regexp.MustCompile("^0x[0-9a-fA-F]{40}$")
return re.MatchString(address)
}

func checkIfUrlIsValid(rawUrl string) error {
// Regular expression to validate URLs
urlPattern := regexp.MustCompile(`^(https?|ftp)://[^\s/$.?#].[^\s]*$`)
Copy link
Contributor

Choose a reason for hiding this comment

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

this regex allows invalid uri
https://a.
https://a.com.
https://1.

It also doesn't allow working with ftps, or other uri schema such as grpc

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea this is not perfect. as in we tailored it to our requirement and allowed certain endpoints, but definitely can add more.


// Check if the URL matches the regular expression
if !urlPattern.MatchString(rawUrl) {
return errors.New("invalid url")
}

parsedURL, err := url.Parse(rawUrl)
if err != nil {
return err
}

// Check if the URL is valid
if parsedURL.Scheme != "" && parsedURL.Host != "" {
return nil
} else {
return errors.New("invalid url")
}
}
5 changes: 4 additions & 1 deletion types/constants.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
package types

const EigenPromNamespace = "eigen"
const (
EigenPromNamespace = "eigen"
MaxNumberOfQuorums = 192
)
25 changes: 5 additions & 20 deletions types/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import (
"math/big"
"net/http"

"github.com/Layr-Labs/eigensdk-go/crypto/bls"
"github.com/Layr-Labs/eigensdk-go/utils"
"github.com/ethereum/go-ethereum/common"

"github.com/Layr-Labs/eigensdk-go/crypto/bls"
)

const (
Expand All @@ -32,15 +32,15 @@ type Operator struct {
}

func (o Operator) Validate() error {
if !utils.IsValidEthereumAddress(o.Address) {
if !isValidEthereumAddress(o.Address) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not preserve the Address type as in go-ethereum ?

Copy link
Contributor

Choose a reason for hiding this comment

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

How many times the Validate function will be called?
If it will be called many times, think of adding an indication if the address was validated already.

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a typo at line 22 (addres)

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's a good point. this PR is getting stale. might as well do in new PR. I will make an issue.

return errors.New("invalid operator address")
}

if !utils.IsValidEthereumAddress(o.EarningsReceiverAddress) {
if !isValidEthereumAddress(o.EarningsReceiverAddress) {
return errors.New("invalid EarningsReceiverAddress address")
}

if o.DelegationApproverAddress != ZeroAddress && !utils.IsValidEthereumAddress(o.DelegationApproverAddress) {
if o.DelegationApproverAddress != ZeroAddress && !isValidEthereumAddress(o.DelegationApproverAddress) {
return fmt.Errorf(
"invalid DelegationApproverAddress address, it should be either %s or a valid non zero ethereum address",
ZeroAddress,
Expand Down Expand Up @@ -108,21 +108,6 @@ type OperatorAvsState struct {
BlockNumber BlockNum
}

var (
maxNumberOfQuorums = 192
)

func BitmapToQuorumIds(bitmap *big.Int) []QuorumNum {
// loop through each index in the bitmap to construct the array
quorumIds := make([]QuorumNum, 0, maxNumberOfQuorums)
for i := 0; i < maxNumberOfQuorums; i++ {
if bitmap.Bit(i) == 1 {
quorumIds = append(quorumIds, QuorumNum(i))
}
}
return quorumIds
}

type QuorumAvsState struct {
QuorumNumber QuorumNum
TotalStake StakeAmount
Expand Down
23 changes: 0 additions & 23 deletions types/operator_metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"net/url"
"path/filepath"
"regexp"
"strings"
)

Expand Down Expand Up @@ -75,28 +74,6 @@ func (om *OperatorMetadata) Validate() error {
return nil
}

func checkIfUrlIsValid(rawUrl string) error {
// Regular expression to validate URLs
urlPattern := regexp.MustCompile(`^(https?|ftp)://[^\s/$.?#].[^\s]*$`)

// Check if the URL matches the regular expression
if !urlPattern.MatchString(rawUrl) {
return errors.New("invalid url")
}

parsedURL, err := url.Parse(rawUrl)
if err != nil {
return err
}

// Check if the URL is valid
if parsedURL.Scheme != "" && parsedURL.Host != "" {
return nil
} else {
return errors.New("invalid url")
}
}

func isImageURL(urlString string) bool {
// Parse the URL
parsedURL, err := url.Parse(urlString)
Expand Down
53 changes: 1 addition & 52 deletions utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,58 +2,12 @@ package utils

import (
"crypto/ecdsa"
"encoding/json"
"errors"
"log"
"math/big"
"os"
"path/filepath"
"regexp"

gethcommon "github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
"gopkg.in/yaml.v3"
"math/big"
)

func ReadFile(path string) ([]byte, error) {
shrimalmadhur marked this conversation as resolved.
Show resolved Hide resolved
b, err := os.ReadFile(filepath.Clean(path))
if err != nil {
return nil, err
}
return b, nil
}

func ReadYamlConfig(path string, o interface{}) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These methods are used in few of libraries we import eigensdk in. So let's not remove

Copy link
Author

Choose a reason for hiding this comment

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

Aaah makes sense. Also, can we move these functions to a different module like eigenUtils because intuitively utils are for internal use?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ideally all internal stuff would've been in an internal package but we didn't follow that convention.

I am not sure what the ideal way of doing this. Are you suggesting we can have utils and eigenUtils both ? one for internal and one for external?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, this might cause breaking changes for the SDK users though.
Honestly, I don't see what these utils functions really abstract away for users. I believe the SDK should be used for its interfaces and the utils like reading config files should be handled by client apps.
What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea you are right. the only reason I added is that, multiple repository we have use this function and I just wanted to have it once. but I kind of agree it's on client to have these standard utils on their own. they can choose their own way to do this.

Copy link
Author

Choose a reason for hiding this comment

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

@shrimalmadhur if you don't mind can you resolve all the comments which do not need any change? I am a bit confused as to what functions should be added back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

done

Copy link
Author

Choose a reason for hiding this comment

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

This function is removed as well in the initial commit ser!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shrimalmadhur is this good to be merged after we rebase on master?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep. @RohanShrothrium can you rebase and resolve conflicts? and then we can merge this

if _, err := os.Stat(path); errors.Is(err, os.ErrNotExist) {
log.Fatal("Path ", path, " does not exist")
}
b, err := ReadFile(path)
if err != nil {
return err
}

err = yaml.Unmarshal(b, o)
if err != nil {
log.Fatalf("unable to parse file with error %#v", err)
}

return nil
}

func ReadJsonConfig(path string, o interface{}) error {
shrimalmadhur marked this conversation as resolved.
Show resolved Hide resolved
b, err := ReadFile(path)
if err != nil {
return err
}

err = json.Unmarshal(b, o)
if err != nil {
log.Fatalf("unable to parse file with error %#v", err)
}

return nil
}

func EcdsaPrivateKeyToAddress(privateKey *ecdsa.PrivateKey) (gethcommon.Address, error) {
publicKey := privateKey.Public()
publicKeyECDSA, ok := publicKey.(*ecdsa.PublicKey)
Expand All @@ -71,8 +25,3 @@ func RoundUpDivideBig(a, b *big.Int) *big.Int {
res.Div(a, b)
return res
}

func IsValidEthereumAddress(address string) bool {
shrimalmadhur marked this conversation as resolved.
Show resolved Hide resolved
re := regexp.MustCompile("^0x[0-9a-fA-F]{40}$")
return re.MatchString(address)
}