proto: add extra metadata fields to SwapView.Opaque
#4164
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Many of the "opaque" variants of our view messages don't have any extra data other than the raw action. This turns out to be insufficient, because often there's additional data that's practically necessary to understand the public parts of the action. For instance, in this case, the `SwapView`, we want to correctly and nicely render the public parts of the swap, which include: - the input amounts, which are transparent; - the pro-rata output amounts, which are computable given public information. However, this information can't be easily rendered just from the underlying `Swap`, because: - it doesn't contain the asset `Metadata` about the values, only the amount and asset IDs; - it doesn't contain the `BatchSwapOutputData` with the (public) results of the batch swap; - it doesn't have precomputed output values, so the rendering would have to understand and replicate the pro-rata computations. Instead, we should make use of the perspective/view mechanism to augment the `SwapView.Opaque` with that data. To complete this PR, we'll need to: - [ ] Regenerate protos and fix any compile errors in the Rust code. - [ ] Ensure that the tx+perspective => view transformation uses data from the perspective to fill in the new fields (this should be similar to the existing code, but for opaque swaps rather than just visible ones). The web frontend's code for displaying simulated "public views" of the user's transactions can probably make use of this functionality after the first step (since it constructs the simulated view by attenuating the "full view" rather than by regenerating with a fresh perspective), but we should fill in the perspective generation for completeness and to ensure that the data we're adding actually makes sense and is possible to generate from the perspective.
|
Met with @/jessepinho to discuss this changeset - the work seems straightforward & we will revisit once the initial proto/perspective generation changes are made, but these proposed changes seem straightforward to consume on the web side, as expected. |
jessepinho
reviewed
Apr 10, 2024
aubrika
force-pushed
the
swap-view-opaque-data
branch
from
da319a6 to
045d6a0
Compare
aubrika
force-pushed
the
swap-view-opaque-data
branch
from
045d6a0 to
b288996
Compare
cratelyn
reviewed
Apr 22, 2024
this removes a todo comment asking a question. > Not finding a BSOD in the TxP means that whoever generated the > transaction perspective didn't include that data, either because they > didn't have it (eg the transaction was created but never confirmed by > the chain and executed) or because they didn't support it in their > implementation. > > We should handle that by graceful degradation, just not rendering the > BSOD-requiring data if we don't have it. \- #4164 (comment)
erwanor
pushed a commit
that referenced
this pull request
Apr 24, 2024
this removes a todo comment asking a question. > Not finding a BSOD in the TxP means that whoever generated the > transaction perspective didn't include that data, either because they > didn't have it (eg the transaction was created but never confirmed by > the chain and executed) or because they didn't support it in their > implementation. > > We should handle that by graceful degradation, just not rendering the > BSOD-requiring data if we don't have it. \- #4164 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Many of the "opaque" variants of our view messages don't have any extra data other than the raw action. This turns out to be insufficient, because often there's additional data that's practically necessary to understand the public parts of the action.
For instance, in this case, the
SwapView, we want to correctly and nicely render the public parts of the swap, which include:However, this information can't be easily rendered just from the underlying
Swap, because:Metadataabout the values, only the amount and asset IDs;BatchSwapOutputDatawith the (public) results of the batch swap;Instead, we should make use of the perspective/view mechanism to augment the
SwapView.Opaquewith that data.To complete this PR, we'll need to:
The web frontend's code for displaying simulated "public views" of the user's transactions can probably make use of this functionality after the first step (since it constructs the simulated view by attenuating the "full view" rather than by regenerating with a fresh perspective), but we should fill in the perspective generation for completeness and to ensure that the data we're adding actually makes sense and is possible to generate from the perspective.
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: