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

Initial implementation #1

Merged
merged 15 commits into from
Nov 15, 2023
Merged

Initial implementation #1

merged 15 commits into from
Nov 15, 2023

Conversation

miiu96
Copy link
Contributor

@miiu96 miiu96 commented Nov 10, 2023

  • Initial implementation of the chainsimulator binary
  • Add extra endpoint /simulator/generate-blocks/:num-blocks that will generate a provided number of blocks for every shard

@miiu96 miiu96 self-assigned this Nov 10, 2023
@ssd04 ssd04 self-requested a review November 13, 2023 11:10
steps:
- uses: actions/setup-go@v3
with:
go-version: 1.20.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
go-version: 1.20.5
go-version: 1.20.7

to have the same version as in the other flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

)

var (
log = logger.GetOrCreate("indexer")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log = logger.GetOrCreate("indexer")
log = logger.GetOrCreate("chainSimulator")

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

}, nil
}

// ExtendProxyServer will extend the proxy server with extra endponts
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ExtendProxyServer will extend the proxy server with extra endponts
// ExtendProxyServer will extend the proxy server with extra endpoints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines +73 to +75
const (
pathToNodeConfig = "../../../mx-chain-go/cmd/node/config"
pathToProxyConfig = "../../../mx-chain-proxy-go/cmd/proxy/config"
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion to add path to proxy and node config as cli parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is done in the next PR

Comment on lines +30 to +33
cmd := exec.Command("cp", "-r", args.PathToProxyConfig, newConfigsPath)
err := cmd.Run()
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

as suggested above, I think it's not up to this binary to handle node and proxy's configs. Also it seems that here you're just reading the configs so there is no need to copy the proxy config to another location. Or if you really want to copy the configs to a separate temporary location, i think it's better to create a CopyDir function instead of using system binary cp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will refactor this in the next PR because there I implemented a function that will copy a folder of a file


proxyInstance.closableComponents.Add(nodeGroupProc, valStatsProc, nodeStatusProc, bp)

//nodeGroupProc.StartCacheUpdate()
Copy link
Contributor

Choose a reason for hiding this comment

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

uncomment this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need heartbeats here, so I will remove this commented line.

Comment on lines 219 to 220
log.Error("cannot ListenAndServe()", "err", err)
os.Exit(1)
Copy link
Contributor

@ssd04 ssd04 Nov 13, 2023

Choose a reason for hiding this comment

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

return error here? there is already an os.Exit(1) in main.go in case of error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed and changed the log

Comment on lines 224 to 225

func (p *proxy) GetHttpServer() *http.Server {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Comment on lines 79 to 81
//buildInfo, ok := debug.ReadBuildInfo()
//if !ok {
// panic("Can't read BuildInfo")
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

startTime := time.Now().Unix()
roundDurationInMillis := uint64(6000)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion to parametrize this in a next PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's done in the next PR


startTime := time.Now().Unix()
roundDurationInMillis := uint64(6000)
roundsPerEpoch := core.OptionalUint64{
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion to parametrize this in a next PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's done in the next PR

Value: 20,
}

apiConfigurator := api.NewFreePortAPIConfigurator("localhost")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion to parametrize this in a next PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's done in the next PR

}

apiConfigurator := api.NewFreePortAPIConfigurator("localhost")
simulator, err := chainSimulator.NewChainSimulator(os.TempDir(), 3, pathToNodeConfig, startTime, roundDurationInMillis, roundsPerEpoch, apiConfigurator)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion to parametrize number of shards in a next PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's done in the next PR

@@ -0,0 +1,6 @@
package facade

type SimulatorHandler interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

for shardID, nodeAPIInterface := range args.RestApiInterfaces {
cfg.Observers = append(cfg.Observers, &data.NodeData{
ShardId: shardID,
Address: "http://" + nodeAPIInterface,
Copy link
Contributor

Choose a reason for hiding this comment

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

const for "http://" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

}

// CreateProxy will create a new instance of proxy
func CreateProxy(args ArgsProxy) (proxy2.ProxyHandler, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

with a little bit of code refactoring in the proxy repo, we can reuse this construction part. Not for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, will make a task to refactor the proxy main.go file

@miiu96 miiu96 merged commit 40c7112 into main Nov 15, 2023
2 checks passed
@miiu96 miiu96 deleted the initial-binary branch November 15, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants