cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.27k stars 3.63k forks source link

gov/v1beta1/proposal doesn't handle correctly new SoftwareUpgradeProposal #14334

Closed robert-zaremba closed 1 year ago

robert-zaremba commented 1 year ago

Summary of Bug

ping.pub and Kepler explorer crash when opening a governance page, when there is a new SoftwareUpgradeProposal (submitted as a new tx gov submit-proposal) as well as any new proposal gov message. The exception in the browser console is: TypeError: t.p.contents is null. Example. Probably other explorers (mintscan...) will crash as well.

The reason is that gov/v1beta1/proposals endpoint doesn't set Content for the new Proposal types. The problem is that today, explorers use gov/v1beta1/proposals and expect Proposal.Content.

Ecosystem bugs:

Version

v0.46.7 I guess, v0.47-alpha2 has same issue.

Steps to Reproduce

  1. Start simd

  2. Make proposal upgrade:

    simd tx gov submit-proposal p.json \
      --from w1 --chain-id $CHAINID  -y --keyring-backend test --home $APP_HOME --fees 5000$DENOM -b block
  3. wait until upgrade, and start new binary with compiled upgrade plan, and API server enabled

  4. Open ping pug gov page - it will break

  5. You won't be able to see details of any new gov proposal, until explorer will start supporting gov/v1

Open 127.0.0.1:1317/cosmos/gov/v1/proposals/1, you will see correct response:

{
  "proposal": {
    "id": "1",
    "messages": [
      {
        "@type": "/cosmos.upgrade.v1beta1.MsgSoftwareUpgrade",
        "authority": "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn",
        "plan": {
          "name": "v3.0.0",
          "time": "0001-01-01T00:00:00Z",
          "height": "45",
          "info": "",
          "upgraded_client_state": null
        }
      }
    ],
    "status": "PROPOSAL_STATUS_PASSED",
    "final_tally_result": {
      "yes_count": "1090000000000",
      "abstain_count": "0",
      "no_count": "0",
      "no_with_veto_count": "0"
    },
    "submit_time": "2022-12-15T23:20:00.636433951Z",
    "deposit_end_time": "2022-12-15T23:21:00.636433951Z",
    "total_deposit": [
      {
        "denom": "stake",
        "amount": "10000000"
      }
    ],
    "voting_start_time": "2022-12-15T23:20:00.636433951Z",
    "voting_end_time": "2022-12-15T23:20:20.636433951Z",
    "metadata": ""
  }
}

open 127.0.0.1:1317/cosmos/gov/v1beta1/proposals/1, you will see response with null content

{
  "proposal": {
    "proposal_id": "1",
    "content": null,
    "status": "PROPOSAL_STATUS_PASSED",
    "final_tally_result": {
      "yes": "1090000000000",
      "abstain": "0",
      "no": "0",
      "no_with_veto": "0"
    },
    "submit_time": "2022-12-15T23:20:00.636433951Z",
    "deposit_end_time": "2022-12-15T23:21:00.636433951Z",
    "total_deposit": [
      {
        "denom": "stake",
        "amount": "10000000"
      }
    ],
    "voting_start_time": "2022-12-15T23:20:00.636433951Z",
    "voting_end_time": "2022-12-15T23:20:20.636433951Z"
  }
}
robert-zaremba commented 1 year ago

In fact the same problem is with all "new" proposals. For example, we created a "new" proposal for one of our custom modules, and it also breaks the explorer, because the content is missing.

tac0turtle commented 1 year ago

@amaurym can lend some help, but v1 is a breaking change and was expected to cause some breaking changes. We can look into what can be done, but also a nil check on explorers can be done.

julienrbrt commented 1 year ago

Changes for gov v1 always needed to be implemented by explorers. For instance, they need to implement metadata changes: https://github.com/cosmos/cosmos-sdk/issues/11301

dogemos commented 1 year ago

Keplr Dashboard already supports the v1beta1 endpoint. Released an update allowing content to be null so this should be no issue on the Keplr side now.

robert-zaremba commented 1 year ago

Great news @dogemos . Is there a roadmap for gov/v1 support?

robert-zaremba commented 1 year ago

BTW, @dogemos , I don't think handling null content is the final solution. Users won't see proposal (name, description, ...) details unless you start supporting gov/v1.

dogemos commented 1 year ago

Hey Robert, Noted. I think I misunderstood the issue as being an endpoint formatting issue. Seems like this is more around the breaking changes on gov/v1. This will take a little bit more time, but I've notified the team and we will work on getting gov/v1 supported.

minxylynx commented 1 year ago

I think the biggest issue is that the gov/v1 stuff no longer has a static title and description. It relies on the goodwill of the proposer to format and upload the Metadata matching the "recommended" format.

Just something to keep in mind, as I've had to deal with the switch as well.

robert-zaremba commented 1 year ago

I am preparing a fix for v1beta1. However, nevertheless let's start supporting v1 endpoints.

robert-zaremba commented 1 year ago

I think the biggest issue is that the gov/v1 stuff no longer has a static title and description. It relies on the goodwill of the proposer to format and upload the Metadata matching the "recommended" format.

Good observation @minxylynx , it's a big issue. The Title / Description is ensured on the interface level, which obviously won't work on the proto level.

minxylynx commented 1 year ago

I think the biggest issue is that the gov/v1 stuff no longer has a static title and description. It relies on the goodwill of the proposer to format and upload the Metadata matching the "recommended" format.

Good observation @minxylynx , it's a big issue. The Title / Description is ensured on the interface level, which obviously won't work on the proto level.

I spent about 2 weeks updating the Provenance Explorer to handle the new changes. That was a big point of frustration. If the metadata doesnt exist, there is no title or description. So I had to handle that case with defaults, which are not a nice as a true title.

I understand why it was moved into a freeform field, but that leaves it open to being ignored.

On the same note, I do parse out the legacy content, if present, to pull the old title and description fields. That at least works for some of the time.

robert-zaremba commented 1 year ago

yeah, I spent this week debuging various things, previously we spent few weeks doing migrations and creating new v1 proposals, so I understand your pain. I would like to avoid rolling back that work.

liangping commented 1 year ago

@robert-zaremba I think we have patched the null content issue, You need put a sdk version >0.46.6 in the chain.json. let me know if it still not work.

Regarding the metadata, We haven't figured out what is a practical way to implement, Because there are some common issues (availability, cors) on parsing contents the external link.

The only thing I would suggest is putting title out of the metadata, using a separated field and storing it on chain. that keep minimum necessary info about the proposal. also help display on front-end, especially for iteration.

robert-zaremba commented 1 year ago

@liangping how does 0.46.7 change anything here (compared to 46.0)?

robert-zaremba commented 1 year ago

@liangping if you don't support gov/v1 endpoint then you won't show title and description (however I've prepared a patch to handle that for messages with explicit title and description content).

liangping commented 1 year ago

we only handle null content when version> 0.46.6, Because osmosis(current 0.46.1) adapted the gov to previous version, while others did not. so we added a version compare.

liangping commented 1 year ago

@liangping if you don't support gov/v1 endpoint then you won't show title and description (however I've prepared a patch to handle that for messages with explicit title and description content).

Welcome to push a request.

hxrts commented 1 year ago

fyi, the reason it title/description was removed was because groups was added and that needed to be off-chain so it didn't blow up state. gov was changed so gov/groups would conform.

We did our best to document this and I went around to major wallets/explorers to notify them of the change months ago.

robert-zaremba commented 1 year ago

@hxrts group/v1.Proposal != gov/v1.Proposal. In fact, group proto doesn't even import gov.

robert-zaremba commented 1 year ago

Is there any explorer which already handle gov metadata? cc: @dogemos , @liangping ... (can anyone ping mintscan, bigdipper, atomscan.com ....)?

liangping commented 1 year ago

Is there any explorer which already handle gov metadata? cc: @dogemos , @liangping ... (can anyone ping mintscan, bigdipper, atomscan.com ....)?

Haven't.

minxylynx commented 1 year ago

@robert-zaremba

https://explorer.provenance.io/dashboard https://github.com/provenance-io/explorer-service/blob/main/service/src/main/kotlin/io/provenance/explorer/service/GovService.kt#L729-L746

i handle it in a generic way, for now

carameleon commented 1 year ago

Mintscan is looking into it and will prepare against this issues.

thanks for ping us. @hxrts

Is there any explorer which already handle gov metadata? cc: @dogemos , @liangping ... (can anyone ping mintscan, bigdipper, atomscan.com ....)?