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

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

Closed
robert-zaremba opened this issue Dec 16, 2022 · 23 comments · Fixed by #14347
Closed

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

robert-zaremba opened this issue Dec 16, 2022 · 23 comments · Fixed by #14347
Labels

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Dec 16, 2022

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:

  • It makes one of the most important features: gov almost unusable without a frontend.
  • Without that, literally nobody should use new gov system (they should be marked experimental, until ecosystem adoption is achieved, rather then deprecating the "working" solutions).
  • We need an ecosystem push for tooling update.

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
Copy link
Collaborator Author

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
Copy link
Member

@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
Copy link
Member

Changes for gov v1 always needed to be implemented by explorers. For instance, they need to implement metadata changes: #11301

@julienrbrt julienrbrt removed the T:Bug label Dec 16, 2022
@dogemos
Copy link
Member

dogemos commented Dec 16, 2022

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
Copy link
Collaborator Author

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

@robert-zaremba
Copy link
Collaborator Author

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
Copy link
Member

dogemos commented Dec 16, 2022

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
Copy link

minxylynx commented Dec 16, 2022

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
Copy link
Collaborator Author

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

@robert-zaremba
Copy link
Collaborator Author

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
Copy link

minxylynx commented Dec 16, 2022

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
Copy link
Collaborator Author

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
Copy link

@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
Copy link
Collaborator Author

robert-zaremba commented Dec 16, 2022

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

@robert-zaremba
Copy link
Collaborator Author

@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
Copy link

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
Copy link

@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
Copy link
Contributor

hxrts commented Dec 17, 2022

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
Copy link
Collaborator Author

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

@robert-zaremba
Copy link
Collaborator Author

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

@liangping
Copy link

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

Haven't.

@carameleon
Copy link

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 ....)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants