-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[bridge] Add print registration info in cli #17908
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
is this ready for review? it seems to be against a private branch and I am confused... |
9ca39eb
to
46057eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all good a couple of comments just to show I read the PR
/// Print current registration info | ||
#[clap(name = "print-bridge-registration-info")] | ||
PrintBridgeRegistrationInfo { | ||
#[clap(long = "sui-rpc-url")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that you know that I know that you know... but you can just say long
when the long name matches the field name. Though maybe was on purpose...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix this in a later PR
}) | ||
.collect::<HashMap<_, _>>(); | ||
let mut authorities = vec![]; | ||
for (_, member) in move_type_bridge_committee.member_registration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could work off references if you wanted particularly considering most of the usage is by reference.
Just commenting to be honest, I do not think it matters much given the usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we consume it anyway here
Description
Add a subcommand to display current registration info.
Test plan
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.