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

action create-hardfork cmd + update description fields in all governance action files. #746

Merged
merged 1 commit into from
May 3, 2024

Conversation

CarlosLopezDeLara
Copy link
Contributor

@CarlosLopezDeLara CarlosLopezDeLara commented May 1, 2024

Changelog

- description: |
    Introduces the `governance action create-hardfork` cmd.
# uncomment types applicable to the change:
  type:
  - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - improvement    # QoL changes e.g. refactoring
  # - bugfix         # fixes a defect
  - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Closes #721

How to trust this PR

Run the command

cardano-cli conway governance action create-hardfork \
--mainnet \
--anchor-url example.com \
--anchor-data-hash c7ddb5b493faa4d3d2d679847740bdce0c5d358d56f9b1470ca67f5652a02745 \
--deposit-return-stake-verification-key-file stake.vkey \
--governance-action-deposit 12345 \
--protocol-major-version 10 \
--protocol-minor-version 0 \
--out-file conway-create-hardfork.action

Explore the text envelope en check it with cbor.me

{
    "type": "Governance proposal",
    "description": "Hardfork initiation governance action",
    "cborHex": "84193039581de18f4a3466a404c11eb410313015b88e447d81b60089e25f611600e6058301f6820a00826b6578616d706c652e636f6d5820c7ddb5b493faa4d3d2d679847740bdce0c5d358d56f9b1470ca67f5652a02745"
}
[12345, h'E18F4A3466A404C11EB410313015B88E447D81B60089E25F611600E605', [1, null, [10, 0]], ["example.com", h'C7DDB5B493FAA4D3D2D679847740BDCE0C5D358D56F9B1470CA67F5652A02745']]

or with

cardano-cli conway governance action view --action-file conway-create-hardfork.action
{
    "anchor": {
        "dataHash": "c7ddb5b493faa4d3d2d679847740bdce0c5d358d56f9b1470ca67f5652a02745",
        "url": "example.com"
    },
    "deposit": 12345,
    "governance action": {
        "contents": [
            null,
            {
                "major": 10,
                "minor": 0
            }
        ],
        "tag": "HardForkInitiation"
    },
    "return address": {
        "credential": {
            "keyHash": "8f4a3466a404c11eb410313015b88e447d81b60089e25f611600e605"
        },
        "network": "Mainnet"
    }
}

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@CarlosLopezDeLara CarlosLopezDeLara marked this pull request as ready for review May 1, 2024 18:55
@CarlosLopezDeLara CarlosLopezDeLara changed the title initial draft of create-hardfork cmd draft of create-hardfork cmd May 1, 2024
@CarlosLopezDeLara CarlosLopezDeLara changed the title draft of create-hardfork cmd action create-hardfork cmd May 1, 2024
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

@carbolymer
Copy link
Contributor

carbolymer commented May 2, 2024

The windows CI failure: IntersectMBO/cardano-node#5811 (comment) - we can ignore windows GHA, it's fixed in nix already.

Copy link
Contributor

@smelc smelc left a comment

Choose a reason for hiding this comment

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

LGTM 🍾 A few typos in the help needs fixing.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! Feel free to merge after @smelc approves.

@CarlosLopezDeLara CarlosLopezDeLara changed the title action create-hardfork cmd action create-hardfork cmd + update description fields in all governance action files. May 3, 2024
@Jimbo4350 Jimbo4350 self-requested a review May 3, 2024 13:32
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

The commit history needs to be cleaned up. Squashing commits and leaving a commit message like:

Squashed commit of the following:

commit https://github.com/IntersectMBO/cardano-cli/commit/82cc5c1ab7ff50d4072a4e4dea0ba9f6665e04be
Merge: https://github.com/IntersectMBO/cardano-cli/commit/8229fc0c0e3d07f26f1c0707893cf5544746e115 https://github.com/IntersectMBO/cardano-cli/commit/e3dc0a02ffdcd44779a7a7dac99d68d1cc235bdc
Author: Carlos LopezDeLara <carlos.lopezdelara@iohk.io>
Date:   Thu May 2 13:10:52 2024 -0600

    Merge branch 'cl/hardforkinit' of github.com:IntersectMBO/cardano-cli into cl/hardforkinit

commit https://github.com/IntersectMBO/cardano-cli/commit/8229fc0c0e3d07f26f1c0707893cf5544746e115
Author: Carlos LopezDeLara <carlos.lopezdelara@iohk.io>
Date:   Wed May 1 12:09:49 2024 -0600

    action create-hardfork command

    Co-Authored-By: Clément Hurlin <smelc@users.noreply.github.com>

commit https://github.com/IntersectMBO/cardano-cli/commit/e3dc0a02ffdcd44779a7a7dac99d68d1cc235bdc
Author: Carlos LopezDeLara <carlos.lopezdelara@iohk.io>
Date:   Wed May 1 12:09:49 2024 -0600

    action create-hardfork command

    Co-Authored-By: Clément Hurlin <smelc@users.noreply.github.com>

@CarlosLopezDeLara

is not very readable. When you squash commits you need to summarize the changes in those commits in a concise and clear manner.

I also don't know what is happening in 3d8a21f4792c0983b94f493ee064a3203a1c86da.

@smelc
Copy link
Contributor

smelc commented May 3, 2024

@CarlosLopezDeLara> those commits should not be here:

image

Since they have been merged already. I can help you clean this up during our pair programming session 👍

@CarlosLopezDeLara CarlosLopezDeLara force-pushed the cl/hardforkinit branch 2 times, most recently from 6d58209 to 18222b2 Compare May 3, 2024 13:50
- Implements action create-hardfork command
- Update golden files
- Include a description on the "description field" of all governance action commands
- Export a friendly function that returns a ByteString (so not in IO)
- Export buildShelleyAddress
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM!

@CarlosLopezDeLara CarlosLopezDeLara merged commit aae2c20 into main May 3, 2024
16 of 21 checks passed
@CarlosLopezDeLara CarlosLopezDeLara deleted the cl/hardforkinit branch May 3, 2024 14:19
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.

[FR] - action create-hardfork-initiation
4 participants