Remove deprecated position state #763
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
Valentine1898
commented
Mar 14, 2024
Comment on lines
327
to
332
| let positionState = new PositionState({ | ||
| state: PositionState_PositionStateEnum.WITHDRAWN, | ||
| sequence: action.value.sequence, | ||
| }); | ||
| const metadata = getLpNftMetadata(action.value.positionId, positionState); | ||
| await this.indexedDb.saveAssetsMetadata(metadata); |
There was a problem hiding this comment.
By default we save the withdrawn lpnft asset with sequence=0, we must additionally save the withdrawn lpnft asset with the sequence we detect
Comment on lines
-313
to
-326
| #[wasm_bindgen] | ||
| pub fn get_lpnft_asset( | ||
| &mut self, | ||
| position_value: JsValue, | ||
| position_state_value: JsValue, | ||
| ) -> Result<JsValue, Error> { | ||
| utils::set_panic_hook(); | ||
|
|
||
| let position: Position = serde_wasm_bindgen::from_value(position_value)?; | ||
| let position_state = serde_wasm_bindgen::from_value(position_state_value)?; | ||
| let lp_nft = LpNft::new(position.id(), position_state); | ||
| let denom = lp_nft.denom(); | ||
| serde_wasm_bindgen::to_value(&denom) | ||
| } |
There was a problem hiding this comment.
This is not part of the view server actually, moved to dex.rs
| ]; | ||
|
|
||
| for (const tx of txs) { | ||
| for (const { action } of tx.body?.actions ?? []) { | ||
| if (action.case === 'positionOpen' && action.value.position) { | ||
| for (const state of positionStates) { | ||
| const positionState = new PositionState({ state }); | ||
| const metadata = this.viewServer.getLpNftMetadata(action.value.position, positionState); | ||
| const metadata = getLpNftMetadata(computePositionId(action.value.position), state); |
There was a problem hiding this comment.
Now getLpNftMetadata() only requires PositionId, not Position. This makes its use more versatile
grod220
approved these changes
Mar 14, 2024
packages/wasm/src/dex.ts
Outdated
Comment on lines
19
to
20
| const result = get_lpnft_asset(positionId.toJson(), positionState.toJson()) as JsonValue; | ||
| return Metadata.fromJsonString(JSON.stringify(result)); |
There was a problem hiding this comment.
Do we have to stringify? Can we not do:
return Metadata.fromJson(result);
There was a problem hiding this comment.
You're right, it works without stringify
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.
This fixes the errors during synchronization that I discovered
Related to penumbra-zone/penumbra#3883
The issue is that
PositionState_PositionStateEnum.CLAIMEDwas marked as deprecated in proto, yet was removed from the rust codebase. This was causing wasm to error and stop synchronization, which caused us to not save the block completely.