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

proto bumps for MsgCancelUnbondingDelegation #9284

Merged
merged 4 commits into from
Apr 25, 2024
Merged

proto bumps for MsgCancelUnbondingDelegation #9284

merged 4 commits into from
Apr 25, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 24, 2024

Description

Makes the proto updating mechanism more robust, getting the latest definitions used in the SDK branch. PTAL @JimLarson

Also adopts the new MsgCancelUnbondingDelegation that is available. PTAL @0xpatrickdev

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

More automated now

Testing Considerations

Manual

Upgrade Considerations

Improves sync of cosmic-proto package with cosmic-swingset

Copy link

cloudflare-workers-and-pages bot commented Apr 24, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 94334b3
Status: ✅  Deploy successful!
Preview URL: https://7f411fc9.agoric-sdk.pages.dev
Branch Preview URL: https://ta-proto-bumps.agoric-sdk.pages.dev

View logs

Copy link
Contributor

@JimLarson JimLarson left a comment

Choose a reason for hiding this comment

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

Great! This is probably sufficient for #9018 too.

Comment on lines +19 to +22
# clean up what we don't need
chmod -R u+w proto
rm -rf proto/cosmos/app
rm -rf proto/cosmos/orm
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of stuff in there that you probably don't need, and you might stub your toe a few times when needs change, but it's your call.

@@ -8,7 +8,7 @@ The generated code is determined by the contents of `protos` and the config of `

## Maintaining protos

The `protos` are held in this source tree and updated from `golang/cosmos/proto` during `yarn codegen` by `yarn protos-update`. The `cosmos` protos were sourced from [@protobufjs/cosmos](https://www.npmjs.com/package/@protobufs/cosmos) per [Telescope's instructions](https://github.com/cosmology-tech/telescope?tab=readme-ov-file#add-protobufs). However we don't use that as a dependency of the package because we need a more manual approach to merge with the Golang-managed protos in the repo.
The `protos` are held in this source tree and updated from `golang/cosmos/proto` during `yarn codegen` by the `update-protos.sh` script.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ambiguous whether yarn codegen invokes update-protos.sh or vice-versa. Make it clear that you call yarn codegen and consider letting folks just read the implementation if they want to see how it works.

I've updated the instructions on the Upgrading the Interchain Stack wiki page.

# update proto files in this package
cp -rf "$COSMOS_SDK"/proto/cosmos proto
cp -rf "$AG_SDK"/golang/cosmos/third_party/proto .
cp -rf "$AG_SDK"/golang/cosmos/proto/agoric proto
Copy link
Member

Choose a reason for hiding this comment

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

LGTM. Seems this should preserve protos added manually (like /icq/v1) that are not part of /golang/cosmos/third_party/proto

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Apr 25, 2024
@mergify mergify bot merged commit 841f246 into master Apr 25, 2024
75 checks passed
@mergify mergify bot deleted the ta/proto-bumps branch April 25, 2024 01:42
mergify bot added a commit that referenced this pull request Apr 25, 2024
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

<!-- Most PRs should close a specific Issue. All PRs should at least
reference one or more Issues. Edit and/or delete the following lines as
appropriate (note: you don't need both `refs` and `closes` for the same
one): -->

Like #9284, but also adds `ibc` protos from `ibc-go/v6`. Motivated by
needing `MsgTransfer` for #9193.

closes: #XXXX
refs: #9193

## Description

<!-- Add a description of the changes that this PR introduces and the
files that
are the most critical to review.
-->

### Security Considerations

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

Added `ibc` to the snapshot tests for this package.

### Upgrade Considerations

<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->

Unlike the `COSMOS_SDK` variable in `update-protos.sh`, `IBC_GO`
contains a version (v6) in the path which may require someone to update
the script when the `cosmos/ibc-go` version is bumped. If someone wants
to suggest a different approach that uses regex, that's more than
welcome.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants