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

analyzer/consensus: Support ParametersChange proposal #573

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Nov 25, 2023

The tesnet proposal_id=9 on live deployments (that are already past the proposal height) should probably just be manually inserted in the DB.

TODO:

  • test (on testnet)
select * from chain.proposals where id = 9;

 id |                   submitter                    | state  | executed |    deposit     | handler | cp_target_version | rhp_target_version | rcp_target_version | upgrade_epoch | cancels | created_at | closes_at | invalid_votes | parameters_change_module |                          parameters_change
----+------------------------------------------------+--------+----------+----------------+---------+-------------------+--------------------+--------------------+---------------+---------+------------+-----------+---------------+--------------------------+----------------------------------------------------------------------
  9 | oasis1qrx85mv85k708ylww597rd42enlzhdmeu56wqj72 | active | f        | 10000000000000 |         |                   |                    |                    |               |         |      30693 |     30705 |             0 | governance               | \xa1781d757067726164655f63616e63656c5f6d696e5f65706f63685f646966660f
(1 row)

@ptrus ptrus force-pushed the ptrus/feature/governance-parameter-change branch 3 times, most recently from badd77e to 1a0d49b Compare November 25, 2023 20:25
submission.State.String(),
submission.Deposit.String(),
submission.Content.ChangeParameters.Module,
submission.Content.ChangeParameters.Changes,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes is cbor.RawMessage of one of the various ConsensusParameterChanges types.

We could probably try unmarshaling this into var changes map[string]interface{} and then marshaling it into JSON, and storing a JSONB. I think that should work in all cases that we are likely to ever hit, but not absolutely certain if there are any potential edge cases, so for now opted to just storing the raw value. The frontend or the API controller could still do this.

@ptrus ptrus marked this pull request as ready for review November 26, 2023 08:24
@ptrus ptrus force-pushed the ptrus/feature/governance-parameter-change branch from 1a0d49b to f34a365 Compare November 28, 2023 09:38
Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting to be one fat table, this :)

Thank you! LGTM assuming you agree with the type change (see comments).

FYI keep an eye out on #574 in case it goes in before yours.

// 5 hardcoded NULLs for the proposal.Content.Upgrade fields.
// 1 hardocded NULL for the proposal.Content.CancelUpgrade.ProposalID field.
proposal.Content.ChangeParameters.Module,
hex.EncodeToString(proposal.Content.ChangeParameters.Changes),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 one day we'll use a sql library in genesis processing too ... it's just not justifiable to spend a day doing that busy work.

The name of the module whose parameters are to be changed
by this 'parameters_change' proposal.
parameters_change:
type: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be preferable to also have format: byte here.
Examples:

format: byte # means base64-encoded raw bytes

nexus/api/spec/v1.yaml

Lines 2295 to 2303 in 6b2fe2e

runtime_bytecode:
type: string
format: byte
description: |
The runtime bytecode of the smart contract. This is the code stored on-chain that
describes a smart contract. Every contract has this info, but Nexus fetches
it separately, so the field may be missing for very fresh contracts (or if the fetching
process is stalled).
gas_used:

Then our openapi generator will represent it as a simple []byte on the Go side, which is more truthful. And when serializing to JSON, it will automatically base64 encode it because that's what the openapi standard prescribes. So you'll be able to drop the ugly manual transformation (p.ParametersChange = common.Ptr(base64.StdEncoding.EncodeToString(parametersChange)) and related lines).

Copy link
Member Author

@ptrus ptrus Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update, thanks 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! oh alternatively, I like the JSON solution (that you mention in the PR description) also

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used the raw cbor message (bytes) for now.

@mitjat mitjat mentioned this pull request Nov 29, 2023
@Andrew7234
Copy link
Collaborator

keep an eye out on #574 in case it goes in before yours.

I'll hold off on merging 574 until this and #540 go in in an effort to minimize the number of migrations 😄

@ptrus ptrus force-pushed the ptrus/feature/governance-parameter-change branch from f34a365 to 246651f Compare November 30, 2023 14:57
@ptrus
Copy link
Member Author

ptrus commented Nov 30, 2023

keep an eye out on #574 in case it goes in before yours.

I'll hold off on merging 574 until this and #540 go in in an effort to minimize the number of migrations 😄

Thanks, merging this now!

@ptrus ptrus merged commit f495dac into main Nov 30, 2023
6 checks passed
@ptrus ptrus deleted the ptrus/feature/governance-parameter-change branch November 30, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants