Fix idb account contains asset, fix wasm gas prices #1603
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@penumbra-zone/getters': minor | ||
| --- | ||
|
|
||
| add getAmountFromNote |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@penumbra-zone/storage': major | ||
| '@penumbra-zone/types': major | ||
| --- | ||
|
|
||
| implement accountHasSpendableAsset correctly |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@penumbra-zone/wasm': minor | ||
| --- | ||
|
|
||
| do not fail planning upon undefined gas |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import { Note } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/component/shielded_pool/v1/shielded_pool_pb.js'; | ||
| import { createGetter } from './utils/create-getter.js'; | ||
|
|
||
| export const getAmountFromNote = createGetter((note?: Note) => note?.value?.amount); |
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,22 +7,29 @@ import { TransactionPlannerRequest } from '@buf/penumbra-zone_penumbra.bufbuild_ | |
| import { fvkCtx } from '../../ctx/full-viewing-key.js'; | ||
| import { extractAltFee } from '../fees.js'; | ||
| import { assertTransactionSource } from './assert-transaction-source.js'; | ||
| import { AddressIndex } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/keys/v1/keys_pb.js'; | ||
| import { toPlainMessage } from '@bufbuild/protobuf'; | ||
|
|
||
| export const transactionPlanner: Impl['transactionPlanner'] = async (req, ctx) => { | ||
| assertValidRequest(req); | ||
|
|
||
| const services = await ctx.values.get(servicesCtx)(); | ||
| const { indexedDb } = await services.getWalletServices(); | ||
|
|
||
| const noNativeTokenAvailable = !(await indexedDb.accountHasSpendableAsset( | ||
| toPlainMessage(req.source ?? new AddressIndex()), | ||
| indexedDb.stakingTokenAssetId, | ||
| )); | ||
|
Comment on lines
+19
to
+22
There was a problem hiding this comment. actually shouldn't |
||
|
|
||
| const planningSwapClaim = req.swapClaims.length > 0; | ||
| // Check if we should use the native token or extract an alternate gas fee token. | ||
| // Special cased for swap claims as gas fee needs to match the claimFee on the corresponding swap. | ||
| console.log('alt fee reasons', noNativeTokenAvailable, planningSwapClaim); | ||
|
|
||
|
|
||
| const gasFeeToken = | ||
| noNativeTokenAvailable || planningSwapClaim | ||
|
There was a problem hiding this comment. I actually prefer keeping |
||
| ? await extractAltFee(req, indexedDb) | ||
| : indexedDb.stakingTokenAssetId; | ||
|
|
||
| const fmdParams = await indexedDb.getFmdParams(); | ||
| if (!fmdParams) { | ||
|
|
@@ -37,12 +44,6 @@ export const transactionPlanner: Impl['transactionPlanner'] = async (req, ctx) = | |
| throw new ConnectError('ChainId not available', Code.FailedPrecondition); | ||
| } | ||
|
|
||
| const idbConstants = indexedDb.constants(); | ||
| const fvk = await ctx.values.get(fvkCtx)(); | ||
| const plan = await planTransaction(idbConstants, req, fvk, gasFeeToken); | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,8 +63,8 @@ import { | |
| DutchAuctionDescription, | ||
| } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/component/auction/v1/auction_pb.js'; | ||
| import { ChainRegistryClient } from '@penumbra-labs/registry'; | ||
| import { PartialMessage, PlainMessage, toPlainMessage } from '@bufbuild/protobuf'; | ||
| import { getAmountFromNote } from '@penumbra-zone/getters/note'; | ||
| import { isZero } from '@penumbra-zone/types/amount'; | ||
| import { IDB_VERSION } from './config.js'; | ||
|
|
||
|
|
@@ -869,20 +869,26 @@ export class IndexedDb implements IndexedDbInterface { | |
| }; | ||
| } | ||
|
|
||
| async accountHasSpendableAsset( | ||
| { account }: PlainMessage<AddressIndex>, | ||
| { inner }: PlainMessage<AssetId>, | ||
| ): Promise<boolean> { | ||
| // asserts length of assetId.inner | ||
| bech32mAssetId({ inner }); | ||
|
|
||
| const assetNotes = ( | ||
| await this.db.getAllFromIndex('SPENDABLE_NOTES', 'assetId', uint8ArrayToBase64(inner)) | ||
| ).map(note => SpendableNoteRecord.fromJson(note)); | ||
|
|
||
| const spendableNotes = assetNotes.filter(({ note, addressIndex, heightSpent }) => { | ||
| const noteAddressIndex = toPlainMessage(addressIndex ?? new AddressIndex()); | ||
| const accountMatch = noteAddressIndex.account === account; | ||
|
There was a problem hiding this comment. is it correct to ignore account randomizer for this comparison? There was a problem hiding this comment. I think we should filter out the randomizer here? https://github.com/penumbra-zone/web/blob/main/packages/wasm/crate/src/planner.rs#L177-L180. There was a problem hiding this comment. the randomizer should be ignored |
||
| const nonzero = !isZero(getAmountFromNote(note)); | ||
| const unspent = heightSpent === 0n; | ||
| return accountMatch && nonzero && unspent; | ||
| }); | ||
|
|
||
| console.log('accountHasSpendableAsset', account, bech32mAssetId({ inner }), spendableNotes); | ||
| return spendableNotes.length > 0; | ||
| } | ||
| } | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,7 +205,7 @@ pub async fn plan_transaction_inner<Db: Database>( | |
| let gas_prices = storage | ||
| .get_gas_prices_by_asset_id(&fee_asset_id) | ||
| .await? | ||
| .unwrap_or_default(); | ||
|
There was a problem hiding this comment. see comment below addressing #1602 There was a problem hiding this comment. I think it's a mistake, we shouldn't use default gas prices There was a problem hiding this comment. I agree with this sentiment. For example, this could theoretically allow paying fees with unsupported gas tokens, like |
||
|
|
||
| let fee_tier = match request.fee_mode { | ||
| None => FeeTier::default(), | ||
|
|
||
Oops, something went wrong.
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.
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.
someone should fix these tests
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.
af28b4d