feat: #1989: subaccount filter in ownedPositionIds
#2006
Conversation
…thod in viewService
| #[wasm_bindgen] | ||
| pub fn is_controlled_address(&self, address: &[u8]) -> WasmResult<bool> { | ||
| pub fn get_index_by_address(full_viewing_key: &[u8], address: &[u8]) -> WasmResult<JsValue> { |
There was a problem hiding this comment.
question: what's the functional difference between this and the other get_index_by_address method in the keys dir – are these not duplicates?
There was a problem hiding this comment.
inspecting the wasm/index.d.ts, it seems like the caller getAddressIndexByAddress is using the get_index_by_address method in keys
There was a problem hiding this comment.
yes, that's right. The block processor uses view server implementation from wasm, so i tried to stick to it without importing methods from other parts of the code
There was a problem hiding this comment.
block processor calls getIndexByAddress, which internally calls getAddressIndexByAddress view service method, which calls get_index_by_address, which is agnostic to which file the wasm method lives in because it's calling the method from ../wasm/index.js which is a global view of all wasm_bindgen methods.
There was a problem hiding this comment.
basically hinting at that we should remove get_index_by_address from view_server.rs. I bring this up because we should ensure that all wasm method names are unique and avoid decorating multiple methods with wasm_bindgen that share the same name.
There was a problem hiding this comment.
agree, this change wasn't really needed. Removed all WASM code updates in favor of the old version
| @@ -496,13 +498,13 @@ describe('IndexedDb', () => { | |||
| it('position should be added and their state should change', async () => { | |||
There was a problem hiding this comment.
suggestion: could we consider adding a few additional tests?
- call
getOwnedPositionIdswith no positions in the database and check it returns an empty array, - check that combining all filters (PositionState, tradingPair, subaccount) works and returns the expected position
- check that subaccount filtering mechanism works when multiple subaccounts exist
There was a problem hiding this comment.
added 1 and 2, number 3 is sufficiently covered by 'should get all position with given subaccount index' test
| ): AsyncGenerator<PositionId, void>; | ||
| addPosition(positionId: PositionId, position: Position): Promise<void>; | ||
| updatePosition(positionId: PositionId, newState: PositionState): Promise<void>; | ||
| addPosition(positionId: PositionId, position: Position, subaccount?: AddressIndex): Promise<void>; |
There was a problem hiding this comment.
ACK subaccount in POSITIONS table
|
note: modifying the db schema requires bumping the |
Closes #1989
Relates to prax-wallet/prax#279 (Prax) and penumbra-zone/dex-explorer#329 (DEX)
Changes:
subaccountfield appears in OwnedPositionIdsResponse bufisControlledAddressmethod togetIndexByAddress. Needed for the block processor to return not just a boolean but the found subaccountgetOwnedPositionIdsmethod. Database should be populated with subaccounts by the latest changes to the block processorownedPositionIdsmethod in ViewServiceNote to reviewers. Before running, call
pnpm gen:penumbrainpackages/protobuf