Conversation
❌ Deploy Preview for cosmos-proposal-builder failed.
|
| maxDepositPeriod: durationToSeconds(params.maxDepositPeriod), | ||
| votingPeriod: durationToSeconds(params.votingPeriod), |
There was a problem hiding this comment.
The api returns the params in snake case
| maxDepositPeriod: durationToSeconds(params.maxDepositPeriod), | |
| votingPeriod: durationToSeconds(params.votingPeriod), | |
| maxDepositPeriod: durationToSeconds(params.max_deposit_period), | |
| votingPeriod: durationToSeconds(params.voting_period), |
michaelfig
force-pushed
the
JeancarloBarrios/adding-govv1-params
branch
from
87e02e1 to
f9ec53d
Compare
src/lib/messageBuilder.ts
Outdated
| import { CommunityPoolSpendProposal } from "cosmjs-types/cosmos/distribution/v1beta1/distribution"; | ||
| import { Params as CosmjsGovV1Params } from "cosmjs-types/cosmos/gov/v1/gov"; | ||
| import { MsgUpdateParams as MsgUpdateGovParamsV1, MsgSubmitProposal } from "cosmjs-types/cosmos/gov/v1/tx"; | ||
| import Long from "long"; |
There was a problem hiding this comment.
This is unidiomatic, use big int for seconds
kriskowal
left a comment
kriskowal
left a comment
There was a problem hiding this comment.
There is a possibility that the attached chain will know about governance parameters that are not present in the Protobuf that Cosgov is compiled against, and since this is downloading the entire parameter state, editing it, then pushing it back, there is a possibility that the new, unknown parameters will be implicitly reset to their zero values.
I am not sure what options we have in that case. If we’re downloading JSON, merging changes into that JSON, and submitting the JSON back in its entirety, there is no problem.
It is possible to write Protobuf stubs that associate parsed messages with aux metadata that includes the unparsed fields, mapping their field number to the corresponding read bytes, and then possible in turn to make the Protobuf writer incorporate the unknown fields. I do not believe that our stubs support this feature. It would be evident in the TypeScript type definition corresponding to the Protobuf message as a bag of unknown fields. So, we might be in a pickle.
It may simply be the case that we need the params parser to fail if it finds unrecognized fields so folks know not to use Cosgov until an upgrade is deployed.
| // Gov v1 Duration type (from protobuf) | ||
| export type Duration = { | ||
| seconds?: string; | ||
| nanos?: number; | ||
| }; |
There was a problem hiding this comment.
This is weird. A single string in nanosecond resolution should suffice to represent a Go uint64-backed Duration and the two-field has multiple representations of the same value. Is this out of our hands?
There was a problem hiding this comment.
I am hoping (and believe) that these types are generated in @agoric/cosmic-proto and it’s a small matter of figuring out the import specifier that reaches them. That’ll mean looking in the "exports" of the package.json of @agoric/cosmic-proto (agoric-sdk/packages/cosmic-proto/package.json) and looking for the module specifier pattern on the left that matches the physical location of the generated types on the right. I believe that’s reaching into dist/*, but those are generated form generated code at src/codegen.
| // Remove duplicate - will use the one at the end | ||
|
|
| const data: GovV1ParamsQueryResponse = await res.json(); | ||
| return data?.params; | ||
| }, | ||
| enabled: !!api, |
There was a problem hiding this comment.
It may be desirable to be more explicit. For the type definition for api, this is equivalent to api !== undefined && api !== '', taking into account the string-to-boolean implicit coercion rules for boolean. I’m wondering whether both terms are intentional. For Protobuf, I would expect empty string to correspond to either “absent on the wire because the sender didn’t know about it, or absent because the sender explicitly set it to empty string”, which are not distinguishable. In JavaScript, I would expect, if the api is optional, that it must be undefined when not provided, and I would assert that the string is valid (not "") if it is provided.
| export const makeGovV1ProposalMsg = ({ | ||
| messages, | ||
| initialDeposit, | ||
| proposer, | ||
| metadata, | ||
| title, | ||
| summary, | ||
| }: MsgSubmitProposal) => ({ | ||
| typeUrl: "/cosmos.gov.v1.MsgSubmitProposal", | ||
| value: MsgSubmitProposal.fromPartial({ | ||
| messages, | ||
| initialDeposit, | ||
| proposer, | ||
| metadata, | ||
| title, | ||
| summary, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
I’m suspicious of the value that these builders are providing. They seem to be the sort of thing that ought to be present in the generated code, or have more precise types than the generated code, or perform some translation, but they do not seem to do any of these things and might as well be inlined.
| import { Params as CosmjsGovV1Params } from "cosmjs-types/cosmos/gov/v1/gov"; | ||
| import { MsgUpdateParams as MsgUpdateGovParamsV1, MsgSubmitProposal } from "cosmjs-types/cosmos/gov/v1/tx"; | ||
| import { Long } from "cosmjs-types/helpers"; |
There was a problem hiding this comment.
I see there’s precedent for using cosmjs-types here, but I suspect (and am uncertain) we could be using the stubs generated for @agoric/cosmic-proto and have a more uniform treatment of Protobuf. The use of Long in particular suggests that these stubs predate the availability of bigint for representing 64 bit integers and beyond on the JavaScript side.
4 participants
No description provided.