fix: empty REST chain-id #1146

mrostamii · 2024-03-11T17:50:58Z

Description

The REST service's initial log had an empty value of chain-id.

Changes

Bugfix (non-breaking change that solves an issue)
Hotfix (change that solves an urgent issue, and requires immediate attention)
New feature (non-breaking change that adds functionality)
Breaking change (change that is not backwards-compatible and/or changes current functionality)
Changes only for a subset of nodes

Checklist

I have added at least 2 reviewer or the whole pos-v1 team
I have added sufficient documentation in code
I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply

Testing

I have added unit tests
I have added tests to CI
I have tested this code manually on local environment
I have tested this code manually on remote devnet using express-cli
I have tested this code manually on mumbai or amoy
I have created new e2e tests into express-cli

Cross repository changes

This PR requires changes to cosmos-sdk

feat/chain flag cosmos-sdk#19

Manual tests

out of this PR

/usr/bin/heimdalld start --chain=mainnet --rest-server
...
INFO [2024-03-11|21:18:02.477] Starting application REST service (chain-id: "")... module=rest-server
...

in this PR

/usr/bin/heimdalld start --chain=mainnet --rest-server
...
INFO [2024-03-11|21:18:02.477] Starting application REST service (chain-id: "mainnet")... module=rest-server
...

- chain-id flag was changed to chain

codecov · 2024-03-12T04:01:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.59%. Comparing base (ae5a160) to head (b971da9).
Report is 27 commits behind head on develop.

❗ Current head b971da9 differs from pull request most recent head 9b2be3f

Please upload reports for the commit 9b2be3f to get more accurate results.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1146      +/-   ##
===========================================
+ Coverage    76.40%   76.59%   +0.19%     
===========================================
  Files           53       53              
  Lines         5929     5922       -7     
===========================================
+ Hits          4530     4536       +6     
+ Misses        1139     1128      -11     
+ Partials       260      258       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marcello33 · 2024-03-17T08:33:18Z

server/root.go

@@ -76,7 +75,7 @@ func StartRestServer(mainCtx ctx.Context, cdc *codec.Codec, registerRoutesFn fun
 	logger.Info(
 		fmt.Sprintf(
 			"Starting application REST service (chain-id: %q)...",
-			viper.GetString(flags.FlagChainID),


I see this value comes from our fork of cosmos-sdk.
I'd probably change the flag value here and raise a PR against maticnetwork/cosmos-sdk

That actually came to my mind too, but didn't want to touch that repo. Btw, let's close this PR once that sdk is modified since that already fixes the issue.

Hey @mrostamii, would you be raising the PR on our cosmos fork ?

@marcello33 @Raneet10 Maybe better to change FlagChainID to FlagChain in both repositories? Tested locally and it was fine, but not sure if it's an accepted strategy.

Yeah that sounds good, please proceed @mrostamii

Hey @mrostamii any update on this? Also, please change target branch to develop. Thank you.

Done.
CC @marcello33 @Raneet10

@mrostamii once the cosmos PR is merged, the version in heimdall should be updated with a new release and this field flags.FlagChainID (now changed to string "chain") needs to be updated to flags.FlagChain

- from heimdall level and cosmos-sdk

mrostamii · 2024-04-12T08:55:47Z

Related PR on cosmos-sdk:

feat/chain flag cosmos-sdk#19

marcello33 · 2024-04-18T09:09:21Z

Thanks @mrostamii
The build seems to be broken, can you please make sure the CI runs successfully?
We'll be reviewing the changes then

mrostamii · 2024-04-18T09:11:56Z

@marcello33 CI got failed because cosmos-sdk must be updated first.

marcello33 · 2024-04-18T09:17:36Z

@marcello33 CI got failed because cosmos-sdk must be updated first.

Right, thanks, that's what I mentioned here
Ok will let others review the cosmos one, we can then release a new version and you can update this PR
cc @Raneet10

github-actions · 2024-05-10T00:21:27Z

This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days.

Raneet10 · 2024-05-30T07:05:30Z

@marcello33 CI got failed because cosmos-sdk must be updated first.

Right, thanks, that's what I mentioned here Ok will let others review the cosmos one, we can then release a new version and you can update this PR cc @Raneet10

@mrostamii @marcello33 Have approved the cosmos PR. Feel free to proceed.

marcello33 · 2024-06-07T08:00:31Z

Closing this in favour of #1169

marcello33 · 2024-06-07T09:14:37Z

Hi @mrostamii
I temporarily released a version of cosmos-sdk with your changes, and have prepared a branch for heimdall with the updated code. Then, I ran some tests on a devnet.
Everything seems to be working except for heimdallcli which appears to be broken for the following reason:

ubuntu@...:$ heimdallcli
panic: heimdallcli flag redefined: chain

goroutine 1 [running]:
github.com/spf13/pflag.(*FlagSet).AddFlag(0xc0001afa00, 0xc000341b80)
        /home/ubuntu/go/pkg/mod/github.com/spf13/pflag@v1.0.5/flag.go:848 +0x5fc
github.com/spf13/pflag.(*FlagSet).VarPF(0xc0001afa00, {0x1840f00, 0xc000123ee0}, {0x12c9cb3, 0x5}, {0x0, 0x0}, {0xc000044b40, 0x32})
        /home/ubuntu/go/pkg/mod/github.com/spf13/pflag@v1.0.5/flag.go:831 +0x105
github.com/spf13/pflag.(*FlagSet).VarP(...)
        /home/ubuntu/go/pkg/mod/github.com/spf13/pflag@v1.0.5/flag.go:837
github.com/spf13/pflag.(*FlagSet).StringVarP(...)
        /home/ubuntu/go/pkg/mod/github.com/spf13/pflag@v1.0.5/string.go:42
github.com/spf13/pflag.(*FlagSet).String(0xc0001afa00, {0x12c9cb3, 0x5}, {0x0, 0x0}, {0xc000044b40, 0x32})
        /home/ubuntu/go/pkg/mod/github.com/spf13/pflag@v1.0.5/string.go:60 +0xb3
github.com/maticnetwork/heimdall/helper.DecorateWithHeimdallFlags(0x20d1ac0, 0xc000304380, {0x18452e0, 0xc000122040}, {0x12c92bb, 0x4})
        /home/ubuntu/matic-cli/devnet/code/heimdall/helper/config.go:732 +0x1c25
main.main()
        /home/ubuntu/matic-cli/devnet/code/heimdall/cmd/heimdallcli/main.go:92 +0x155

I believe renaming that var from chain-id to chain makes its name clash with this one.

Please, keep in mind that the commands for heimdalld and heimdallcli are widely used by the community, so we would like not to introduce any breaking change, which is the case with this PR and the one in cosmos.

Also, we were not able to fully understand what issues the empty rest chain-id causes to you, as it is anyway working on all networks, and no error is thrown.
I will need to revert the changes for cosmos-sdk and close this and its twin PRs, as we need to prepare cosmos-sdk and heimdall for the next release.

Thank you for your understanding, and again for your contribution.

marcello33

As per latest comment, heimdallcli to be fixed and conflicts resolved

fix: empty REST chain-id

b971da9

- chain-id flag was changed to chain

pratikspatil024 requested a review from a team March 12, 2024 03:59

marcello33 reviewed Mar 18, 2024

View reviewed changes

marcello33 requested a review from a team March 18, 2024 08:59

mrostamii changed the base branch from master to develop April 11, 2024 16:16

mrostamii added 2 commits April 12, 2024 11:32

Merge branch 'develop' into master

619457e

update: chaid-id flag to chain

9b2be3f

- from heimdall level and cosmos-sdk

mrostamii requested a review from marcello33 April 12, 2024 08:56

mrostamii mentioned this pull request Apr 15, 2024

feat/chain flag maticnetwork/cosmos-sdk#19

Closed

5 tasks

github-actions bot added Stale and removed Stale labels May 10, 2024

marcello33 mentioned this pull request Jun 7, 2024

fix empty chain id #1169

Closed

marcello33 closed this Jun 7, 2024

marcello33 mentioned this pull request Jun 7, 2024

chg: fix heimdall chain id maticnetwork/matic-cli#233

Closed

11 tasks

marcello33 reopened this Jun 7, 2024

marcello33 requested changes Jun 7, 2024

View reviewed changes

marcello33 closed this Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: empty REST chain-id #1146

fix: empty REST chain-id #1146

mrostamii commented Mar 11, 2024 •

edited

codecov bot commented Mar 12, 2024 •

edited

marcello33 Mar 17, 2024

mrostamii Mar 18, 2024

Raneet10 Mar 21, 2024

mrostamii Mar 21, 2024

marcello33 Apr 4, 2024

marcello33 Apr 10, 2024

mrostamii Apr 12, 2024

marcello33 Apr 18, 2024

mrostamii commented Apr 12, 2024

marcello33 commented Apr 18, 2024

mrostamii commented Apr 18, 2024

marcello33 commented Apr 18, 2024

github-actions bot commented May 10, 2024

Raneet10 commented May 30, 2024

marcello33 commented Jun 7, 2024

marcello33 commented Jun 7, 2024 •

edited

marcello33 left a comment

fix: empty REST chain-id #1146

fix: empty REST chain-id #1146

Conversation

mrostamii commented Mar 11, 2024 • edited

Description

Changes

Checklist

Testing

Cross repository changes

Manual tests

codecov bot commented Mar 12, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrostamii commented Apr 12, 2024

marcello33 commented Apr 18, 2024

mrostamii commented Apr 18, 2024

marcello33 commented Apr 18, 2024

github-actions bot commented May 10, 2024

Raneet10 commented May 30, 2024

marcello33 commented Jun 7, 2024

marcello33 commented Jun 7, 2024 • edited

marcello33 left a comment

Choose a reason for hiding this comment

mrostamii commented Mar 11, 2024 •

edited

codecov bot commented Mar 12, 2024 •

edited

marcello33 commented Jun 7, 2024 •

edited