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

refactor(protobuf): authorizeAndBuild method #3618

Merged
merged 1 commit into from
Jan 18, 2024
Merged

refactor(protobuf): authorizeAndBuild method #3618

merged 1 commit into from
Jan 18, 2024

Conversation

TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented Jan 17, 2024

We're eventually deprecating witnessAndBuild in favor of the authorizeAndBuild RPC method, and it unnecessarily requires an authorizationData field in order to be invoked. This modification of the protobuf definition is a necessary precursor for penumbra-zone/web#369.

cc @grod220 @turbocrime

@TalDerei TalDerei requested a review from cronokirby January 17, 2024 07:46
@TalDerei TalDerei requested a review from grod220 January 17, 2024 07:46
@TalDerei TalDerei self-assigned this Jan 17, 2024
@TalDerei TalDerei requested a review from conorsch January 17, 2024 07:56
@TalDerei
Copy link
Collaborator Author

TalDerei commented Jan 17, 2024

@conorsch how can I satisfy the CI here? the breaking change is intentional

@hdevalence
Copy link
Member

Why are we getting rid of this RPC? Just because the web extension isn't planning to use it doesn't mean we should lock users out of a split custody setup...

@TalDerei
Copy link
Collaborator Author

Why are we getting rid of this RPC? Just because the web extension isn't planning to use it doesn't mean we should lock users out of a split custody setup...

good point, I should have clarified. That RPC method won't specifically be used for sending transactions in the web extension. I reverted the change in the web and added comments.

@TalDerei
Copy link
Collaborator Author

@hdevalence should we proceed with this change?

@hdevalence
Copy link
Member

Yep, let's get rid of the erroneous argument I added via copy paste error, but leave the RPC in place.

@conorsch
Copy link
Contributor

how can I satisfy the CI here? the breaking change is intentional

You cannot, the CI is red because it's telling us "this is a breaking change." We know that, and we accept the breaking change and its implications, so what I'll do it override the merge control and get it into main anyway. However, merge is also blocked due to conflicts, so this needs to be freshly rebased on main (at least #3559, merged today, will have introduced conflicts). Can you rebase and mark ready for review?

@conorsch
Copy link
Contributor

Spoke with @TalDerei out of band, I'm going to rebase and get this to green (modulo the proto lint).

@conorsch conorsch marked this pull request as ready for review January 18, 2024 22:08
@conorsch
Copy link
Contributor

CI is good here. I'm holding off on merge so that @TalDerei can perform a bit more testing locally.

@conorsch
Copy link
Contributor

Tal confirmed good to go, so I'm merging to unblock further work in web world.

@conorsch conorsch merged commit 0f53f41 into main Jan 18, 2024
6 of 7 checks passed
@conorsch conorsch deleted the wasm-protobuf branch January 18, 2024 23:45
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