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

POS-15 Remove building Heimdall for each chain #738

Merged
merged 7 commits into from
Nov 9, 2021
Merged

POS-15 Remove building Heimdall for each chain #738

merged 7 commits into from
Nov 9, 2021

Conversation

igorcrevar
Copy link
Contributor

@igorcrevar igorcrevar commented Nov 4, 2021

a new Heimdall binary gets built for each chain using Makefile and including some config parameters at build time.

Build is done with:

https://github.com/maticnetwork/heimdall#usage

Heimdall runs this function to generate the heimdall_params.go:

https://github.com/maticnetwork/heimdall/blob/master/Makefile#L34

https://github.com/maticnetwork/heimdall/blob/master/helper/heimdall-params.template.go

It is only used to set this param for the client:

https://github.com/maticnetwork/heimdall/blob/master/helper/heimdall-params.template.go#L20

The goal is to move those parameters to Heimdall and use a flag:

$ heimdall --chain=[mumbai,mainnet,local]

to run the binary for each chain.

Internally, depending on the chain set the value of the NewSelectionAlgoHeight value.

Proposal:
There are multiple copying of chain toml files (during build and during init), maybe we should hard-code NewSelectionAlgoHeights and create files (or just one toml) during init phase

@ferranbt ferranbt requested a review from vcastellm November 5, 2021 09:34
Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@igorcrevar
Copy link
Contributor Author

I forgot to edit README.md and to remove one line from .gitignore and makefile referencing heimdall-params.go
Also I have added $ heimdalld init between install and start part of readme, it was not there before. Please review these changes also

@ferranbt
Copy link
Contributor

ferranbt commented Nov 7, 2021

LGTM! I will add also Arpit to review since he is the one with the most experience with Heimdall and also so that he can be on the loop.

@codecov-commenter
Copy link

Codecov Report

Merging #738 (f8b5b91) into master (1cb8737) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #738   +/-   ##
=======================================
  Coverage   72.88%   72.88%           
=======================================
  Files          49       49           
  Lines        3507     3507           
=======================================
  Hits         2556     2556           
  Misses        723      723           
  Partials      228      228           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cb8737...f8b5b91. Read the comment docs.

@ferranbt
Copy link
Contributor

ferranbt commented Nov 9, 2021

As commented over Slack, please change the target branch for the PR to v2 and we can merge it.

@igorcrevar igorcrevar changed the base branch from master to v2 November 9, 2021 09:26
@ferranbt ferranbt merged commit b1f2986 into maticnetwork:v2 Nov 9, 2021
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.

5 participants