Fix idb account contains asset, fix wasm gas prices #1603

turbocrime · 2024-07-31T06:24:11Z

this PR corrects idb method used to identify tokens present in an account by correctly comparing address index and asset id. the method will assert the validity of incoming arguments.

this PR corrects wasm planner to use default-zero gas fees if none are present in idb query.

some minifront ui may still be submitting plans with asset id that are not valid.

it should be decided if:

incoming rpc with asset id that do not contain a proper inner binary field should be accepted
incoming rpc with asset id that do not contain a proper inner binary field should be rejected

tested on deimos (zero chain gas fees) successfully with ibc-osmo gda to um and ibc-osmo out.

tested on phobos (chain gas fees) successfully with ibc-osmo gda to um and ibc-osmo out.

ci is failing because tests are using incorrect data.

changeset-bot · 2024-07-31T06:24:14Z

🦋 Changeset detected

Latest commit: af28b4d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages

Name	Type
@penumbra-zone/getters	Minor
@penumbra-zone/storage	Major
@penumbra-zone/types	Major
@penumbra-zone/wasm	Major
minifront	Patch
@penumbra-zone/perspective	Major
@penumbra-zone/query	Major
@penumbra-zone/services	Major
@repo/ui	Patch
node-status	Patch
@penumbra-zone/crypto-web	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

turbocrime · 2024-07-31T06:25:32Z

packages/services/src/view-service/fees.ts

  if (assetId) {
-    const balance = await indexedDb.hasTokenBalance(request.source, assetId);
+    const balance = await indexedDb.accountHasSpendableAsset(request.source, assetId);
    // This check ensures that the alternative fee token is a valid fee token, for example, TestUSD is not.
-    const isInGasTable = altGasPrices.find(gp => gp.assetId?.equals(assetId));
+    const isInGasTable = altGasPrices.find(
+      // TODO: assert this more thoroughly
+      gp => bech32mAssetId(gp.assetId!) === bech32mAssetId(assetId),
+    );
    if (balance && isInGasTable) {
      return assetId;
    }


this part could be improved but i'm going to bed

this checks if the assetId passed in exists in the GasPrices table. since we're comparing assetId objects directly, why isn't the equality method sufficient here?

turbocrime · 2024-07-31T06:34:53Z

packages/storage/src/indexed-db/index.ts

+
+    const spendableNotes = assetNotes.filter(({ note, addressIndex, heightSpent }) => {
+      const noteAddressIndex = toPlainMessage(addressIndex ?? new AddressIndex());
+      const accountMatch = noteAddressIndex.account === account;


is it correct to ignore account randomizer for this comparison?

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.

cc @Valentine1898

the randomizer should be ignored
https://discord.com/channels/824484045370818580/1044457038886998137/1220144758970781707

turbocrime · 2024-07-31T06:39:18Z

packages/services/src/view-service/fees.test.ts

someone should fix these tests

TalDerei · 2024-07-31T07:24:03Z

There are a few points to note about #1602.

For context, GasPrices not Available essentially means that "the fee we're trying to use for the transaction hasn't been defined on-chain and therefore isn't in the GasPrices table, making it unusable for the transaction". That sounds reasonable to me.

1. This situation isn't representative of mainnet conditions because the mainnet chain does have gas prices.

2. If alternative fees in the GasPrices table are undefined, it seems they already default to 0. For example:

Screen.Recording.2024-07-31.at.1.49.27.AM.mov

3. The consequence of the refactor is that it wouldn't even reach that point because it will fail early and be caught by minfront if the user doesn't have a balance for any of the alternative fees defined in the GasPrices table. In the example below, notice that the account doesn't have any UM, GM, or GN, which are the only assets defined in the GasPrices table. Not able to pay fee for transaction is the error we should expect to see.

Screen.Recording.2024-07-31.at.2.14.32.AM.mov

packages/services/src/view-service/fees.ts

TalDerei · 2024-07-31T07:39:42Z

packages/services/src/view-service/transaction-planner/index.ts

+  const noNativeTokenAvailable = !(await indexedDb.accountHasSpendableAsset(
+    toPlainMessage(req.source ?? new AddressIndex()),
+    indexedDb.stakingTokenAssetId,
+  ));


actually shouldn't AddressIndex never be undefined, since it will fail the transaction source check. In that case, why can't we use the non-null assertion operator on source?

TalDerei · 2024-07-31T07:39:50Z

packages/services/src/view-service/transaction-planner/index.ts

-  const gasFeeToken = needsAltFeeToken
-    ? await extractAltFee(req, indexedDb)
-    : indexedDb.stakingTokenAssetId;
+  console.log('alt fee reasons', noNativeTokenAvailable, planningSwapClaim);


TalDerei · 2024-07-31T07:40:45Z

packages/services/src/view-service/transaction-planner/index.ts

+  console.log('alt fee reasons', noNativeTokenAvailable, planningSwapClaim);
+
+  const gasFeeToken =
+    noNativeTokenAvailable || planningSwapClaim


I actually prefer keeping needsAltFeeToken, so we can tack on more invariants later.

TalDerei · 2024-07-31T07:41:32Z

packages/wasm/crate/src/planner.rs

@@ -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?
-        .ok_or_else(|| anyhow!("GasPrices not available"))?;
+        .unwrap_or_default();


see comment below addressing #1602

I think it's a mistake, we shouldn't use default gas prices
And use only those gas prices that have been published in compact blocks

I agree with this sentiment. For example, this could theoretically allow paying fees with unsupported gas tokens, like TestUSD. However, I don't think it would ever error in the planner because we have checks in the frontend at in getAssetFromGasPriceTable, which would then trigger this error.

TalDerei · 2024-07-31T07:46:59Z

packages/storage/src/indexed-db/index.ts

+
+    const spendableNotes = assetNotes.filter(({ note, addressIndex, heightSpent }) => {
+      const noteAddressIndex = toPlainMessage(addressIndex ?? new AddressIndex());
+      const accountMatch = noteAddressIndex.account === account;


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.

cc @Valentine1898

Valentine1898 · 2024-07-31T09:43:18Z

packages/wasm/crate/src/planner.rs

@@ -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?
-        .ok_or_else(|| anyhow!("GasPrices not available"))?;
+        .unwrap_or_default();


I think it's a mistake, we shouldn't use default gas prices
And use only those gas prices that have been published in compact blocks

grod220 · 2024-07-31T10:27:13Z

Taking this over to expedite bug fixes

TalDerei · 2024-07-31T10:35:56Z

prioritizing #1605, this pr can be useful follow-up work.

turbocrime · 2024-08-01T23:15:28Z

update: address-index comparison is corrected by #1605 but remaining issues persist

TalDerei · 2024-08-14T01:17:46Z

closing this per the comments #1603 (comment) and #1603 (comment). This changeset is outdated and introduces unnecessary changes to what appear to be non-issues.

The only remaining open question concerns the representation of the asset Id. I believe that any incoming rpc with an asset Id that lacks a proper inner binary field — which might come from the frontend but it won’t ever come in a compact block — should be rejected. However, if we decide otherwise, we can address this in a separate PR and reduce the scope of the changes.

turbocrime added 3 commits July 30, 2024 22:58

correct gas price selection

b04fb2b

update an extractAltFee helper

fce91c5

update tests

52d24b8

turbocrime changed the title ~~Fix idb~~ Fix idb account contains asset, fix wasm gas prices Jul 31, 2024

turbocrime requested review from TalDerei, grod220 and a team July 31, 2024 06:24

turbocrime commented Jul 31, 2024

View reviewed changes

changeset

7fd2724

turbocrime commented Jul 31, 2024

View reviewed changes

fix gas-prices.test

2b882ce

turbocrime commented Jul 31, 2024

View reviewed changes

TalDerei reviewed Jul 31, 2024

View reviewed changes

fix unit tests

af28b4d

Valentine1898 suggested changes Jul 31, 2024

View reviewed changes

TalDerei assigned TalDerei and turbocrime Aug 3, 2024

TalDerei closed this Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix idb account contains asset, fix wasm gas prices #1603

Fix idb account contains asset, fix wasm gas prices #1603

turbocrime commented Jul 31, 2024 •

edited

Loading

changeset-bot bot commented Jul 31, 2024 •

edited

Loading

turbocrime Jul 31, 2024

TalDerei Jul 31, 2024 •

edited

Loading

turbocrime Jul 31, 2024

TalDerei Jul 31, 2024 •

edited

Loading

Valentine1898 Jul 31, 2024

turbocrime Jul 31, 2024

TalDerei Jul 31, 2024

TalDerei commented Jul 31, 2024 •

edited

Loading

TalDerei Jul 31, 2024

TalDerei Jul 31, 2024

TalDerei Jul 31, 2024

TalDerei Jul 31, 2024

Valentine1898 Jul 31, 2024

TalDerei Jul 31, 2024 •

edited

Loading

TalDerei Jul 31, 2024 •

edited

Loading

Valentine1898 Jul 31, 2024

grod220 commented Jul 31, 2024

TalDerei commented Jul 31, 2024

turbocrime commented Aug 1, 2024

TalDerei commented Aug 14, 2024 •

edited

Loading

Fix idb account contains asset, fix wasm gas prices #1603

Fix idb account contains asset, fix wasm gas prices #1603

Conversation

turbocrime commented Jul 31, 2024 • edited Loading

ci is failing because tests are using incorrect data.

changeset-bot bot commented Jul 31, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

TalDerei Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei commented Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

TalDerei Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 commented Jul 31, 2024

TalDerei commented Jul 31, 2024

turbocrime commented Aug 1, 2024

TalDerei commented Aug 14, 2024 • edited Loading

turbocrime commented Jul 31, 2024 •

edited

Loading

changeset-bot bot commented Jul 31, 2024 •

edited

Loading

TalDerei Jul 31, 2024 •

edited

Loading

TalDerei Jul 31, 2024 •

edited

Loading

TalDerei commented Jul 31, 2024 •

edited

Loading

TalDerei Jul 31, 2024 •

edited

Loading

TalDerei Jul 31, 2024 •

edited

Loading

TalDerei commented Aug 14, 2024 •

edited

Loading