mrostamii commented 6 months ago

Description

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

Changes

[x] 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
[x] 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
[x] 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

https://github.com/maticnetwork/cosmos-sdk/pull/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
...

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mrostamii commented 5 months ago

Related PR on cosmos-sdk:

https://github.com/maticnetwork/cosmos-sdk/pull/19

marcello33 commented 5 months ago

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 commented 5 months ago

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

marcello33 commented 5 months ago

@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[bot] commented 4 months ago

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 commented 4 months ago

@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 commented 3 months ago

Closing this in favour of https://github.com/maticnetwork/heimdall/pull/1169

marcello33 commented 3 months ago

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.

maticnetwork / heimdall

fix: empty REST chain-id #1146