Update core dependencies to v0.70.3 #839

jessepinho · 2024-03-26T22:56:28Z

In this PR

Update Cargo and NPM dependencies on core packages.
Fix various places where we were previously using epochs, but are now using block heights.
- This includes unbonding tokens. Unbonding tokens used to be named like uunbonding_epoch_N_penumbravalid1abc123. Now, unbonding tokens refer to a start block height, rather than a start epoch, so they're of the form unbonding_start_at_N_penumbravalidabc123. Also, they now have multiple denom units, so the display denom just starts with unbonding_ rather than uunbonding.
Fix a bug with how we stored epochs.
- Previously, we stored new epochs only at epoch transitions. However, this meant that the first epoch (index 0) was missing! So, I updated the block processor to create an initial epoch if we're starting sync at block height 0.

packages/wasm/crate/Cargo.toml

TalDerei · 2024-03-27T16:16:02Z

unblocked by v0.70.1 point release

jessepinho · 2024-03-27T22:07:40Z

apps/extension/.env

@@ -1,6 +1,6 @@

 PRAX=lkpmkhpnhknhmibgnmmhdhgdilepfghe
-IDB_VERSION=30
+IDB_VERSION=31


Needed for the update to unbonding tokens, which now have a different symbol

jessepinho · 2024-03-27T22:11:45Z

packages/wasm/crate/Cargo.toml

-penumbra-stake         = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.68.3", package = "penumbra-stake", default-features = false }
-penumbra-tct           = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.68.3", package = "penumbra-tct" }
-penumbra-transaction   = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.68.3", package = "penumbra-transaction", default-features = false }
+penumbra-asset         = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.70.2", package = "penumbra-asset" }


Just updating v0.68.3 to v0.70.2 on all of these

jessepinho · 2024-03-27T22:17:54Z

packages/query/src/block-processor.ts

@@ -273,7 +278,7 @@ export class BlockProcessor implements BlockProcessorInterface {

      const isLastBlockOfEpoch = !!compactBlock.epochRoot;
      if (isLastBlockOfEpoch) {
-        void this.handleEpochTransition(compactBlock.height, latestKnownBlockHeight);
+        await this.handleEpochTransition(compactBlock.height, latestKnownBlockHeight);


Needed to await due to an unlikely, but possible, race condition. When we save a new epoch to the database during an epoch transition, we look in the database for the previous latest known epoch. We grab its epoch index, increment it by 1, then use that index for the new epoch we're saving to the database.

If that took a long time, for some reason, and in the meantime the block processor had already gotten to the next epoch, we could have two epochs with the same epoch index.

This change makes sure that epoch indexes increment in the correct order.

jessepinho · 2024-03-27T22:18:22Z

packages/router/src/grpc/view-protocol-server/asset-metadata-by-id.test.ts

@@ -75,10 +75,11 @@ describe('AssetMetadataById request handler', () => {
    expect(metadataByIdResponse.denomMetadata?.equals(metadataFromNode)).toBeTruthy();
  });

-  test('should fail to get metadata when metadata not found in idb and node', async () => {
+  test('should return an empty response when metadata not found in idb and node', async () => {


There's really no need to throw, since it's acceptable to return an empty response from this RPC method. So I changed the behavior.

Response on other comment

jessepinho · 2024-03-27T22:18:40Z

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

@@ -458,12 +458,14 @@ export class IndexedDb implements IndexedDbInterface {
    });
  }

-  async addEpoch(startHeight: bigint, index?: bigint): Promise<void> {


We're never actually using the index argument anywhere, so I just got rid of it.

jessepinho · 2024-03-27T22:22:18Z

packages/ui/components/ui/tx/view/undelegate.tsx

-          <ActionDetails.Row label='Epoch index'>
-            {value.startEpochIndex.toString()}
-          </ActionDetails.Row>


This is a little strange to me: the startEpochIndex property is deprecated, so we should no longer show it. Ideally, we'd show the unbonding start height instead – but that property doesn't exist on the Undelegate proto! So for now, we'll just hide this, and revisit this if/when core adds that property to the Undelegate action.

can you leave the code here and just style it hidden or comment it out?

jessepinho · 2024-03-27T22:45:26Z

packages/ui/components/ui/tx/view/value/index.tsx

@@ -33,7 +33,7 @@ export const ValueViewComponent = ({
    const value = view.valueView.value;
    const metadata = view.valueView.value.metadata;
    const amount = value.amount ?? new Amount();
-    const exponent = getDisplayDenomExponent(metadata);


Reviewers: I could use some help here.

When a user clicks the Undelegate button, the transaction approval dialog opens. We try to fetch the metadata for the undelegation tokens they'll be getting, but that will always come back empty, since the unbonding token type is now specific to the block height at which unbonding starts. That is, the unbonding token name is unbonding_start_at_N_penumbravalid1..., where N is the block height that unbonding started at.

There will never be token metadata for an unbonding token until after the transaction has been committed to the chain, since the token type is created when the transaction is committed.

But that means that the undelegation tokens are shown as "Unknown asset" in the transaction approval dialog:

Perhaps we should generate the unbonding token metadata on the fly here? If so, could someone point me in the right direction for how to do that? (That said, I'd love to get this PR merged before handling that, since all delegation is blocked until this PR is merged.)

Another wrinkle to all this: this issue, where the metadata request always returns an empty response for an undelegation token that doesn't exist yet, wasn't happening before when undelegation tokens were tied to epochs rather than block heights.

I don't know how that's possible. We should have had the same problem back then, right? That is:

We transition to a new epoch.

Someone goes to undelegate tokens from a validator.

Since no one else has undelegated tokens from that validator in this epoch yet, the metadata for uunbonding_epoch_N_... doesn't exist yet, so the metadata response is empty.

getDisplayDenomExponent throws, because it's being passed undefined metadata.

And yet, this wasn't happening before — no error was being thrown. It only started with this change, where unbonding tokens are tied to block height rather than epoch index.

I'm investigating this, but it's a bit concerning since I don't understand why it worked before!

Perhaps we should generate the unbonding token metadata on the fly here?

The assetMetadataById rpc method feels like a good place to generate metadata for this edge case so web apps have meaningful display data here.

And yet, this wasn't happening before

Umm, no idea! 😅

jessepinho · 2024-03-27T22:53:20Z

packages/wasm/crate/src/planner.rs

        let amount = amount
            .ok_or_else(|| anyhow!("missing amount in delegation"))?
            .try_into()?;
        let rate_data: RateData = rate_data
            .ok_or_else(|| anyhow!("missing rate data in delegation"))?
            .try_into()?;
-        actions.push(rate_data.build_delegate(amount).into());
+        actions.push(rate_data.build_delegate(epoch.into(), amount).into());


The epoch in the rate data has been deprecated, so we have to pass it in now.

jessepinho · 2024-03-27T22:55:43Z

packages/query/src/block-processor.ts

+      // it's done.
+      await this.updateValidatorInfos(0n);
+
+      await this.indexedDb.addEpoch(0n);


Previously, we were skipping saving the first epoch to the database. This fixes that bug.

grod220

Wow, some really domain specific knowledge here 😅 . Good job! Left some comments.

packages/router/src/grpc/view-protocol-server/helpers.ts

grod220 · 2024-03-28T09:00:33Z

packages/router/src/grpc/view-protocol-server/asset-metadata-by-id.test.ts

@@ -75,10 +75,11 @@ describe('AssetMetadataById request handler', () => {
    expect(metadataByIdResponse.denomMetadata?.equals(metadataFromNode)).toBeTruthy();
  });

-  test('should fail to get metadata when metadata not found in idb and node', async () => {
+  test('should return an empty response when metadata not found in idb and node', async () => {


Response on other comment

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

grod220 · 2024-03-28T09:15:13Z

packages/ui/components/ui/tx/view/value/index.tsx

@@ -33,7 +33,7 @@ export const ValueViewComponent = ({
    const value = view.valueView.value;
    const metadata = view.valueView.value.metadata;
    const amount = value.amount ?? new Amount();
-    const exponent = getDisplayDenomExponent(metadata);


Perhaps we should generate the unbonding token metadata on the fly here?

The assetMetadataById rpc method feels like a good place to generate metadata for this edge case so web apps have meaningful display data here.

And yet, this wasn't happening before

Umm, no idea! 😅

packages/wasm/crate/src/storage.rs

grod220

Ah, please also update the proving keys:

web/apps/extension/src/utils/download-proving-keys.ts

Line 7 in a3bb015

const VERSION_TAG = 'v0.68.0';

TalDerei · 2024-03-28T17:08:01Z

Are we waiting for the 0.71.0 point release later today?

jessepinho force-pushed the jessepinho/update-core branch from 39754b8 to 1d7d38e Compare March 27, 2024 00:21

grod220 reviewed Mar 27, 2024

View reviewed changes

packages/wasm/crate/Cargo.toml Outdated Show resolved Hide resolved

This was referenced Mar 27, 2024

Update all non-crate deps #841

Merged

Support v0.70.0 chain upgrade #817

Closed

jessepinho and others added 5 commits March 27, 2024 11:14

Update dependencies

b32c81e

Add epochs to constants

a8127ca

Get the latest epoch from the database, rather than from the rate data

b059118

Add missing properties

c3b06cd

update deps to 0.70.1

d3eb48e

jessepinho force-pushed the jessepinho/update-core branch from 1907382 to d3eb48e Compare March 27, 2024 18:14

jessepinho added 11 commits March 27, 2024 11:58

Change how unbonding tokens are parsed

30ed73e

Fix how unbonding token symbols are displayed

d78eb7c

Update deps

b97cc09

Fix getter name

69a0d12

Fix property

512a1fb

Fix a few bugs with how epochs were stored

d2db570

Fix issues with unbonding token display denom

d0bbdf7

Don't throw when asset metadata doesn't exist

d992488

Handle missing metadata

a952c71

Hide the epoch index of an undelegate action

b333ac7

Fix getter; show unbonding start height for undelegate claims

cdc8e16

jessepinho commented Mar 27, 2024

View reviewed changes

Fix tests

9b48ccb

jessepinho commented Mar 27, 2024

View reviewed changes

jessepinho added 2 commits March 27, 2024 15:13

Tweak var name

01eb8e3

Fix comment

d741b34

jessepinho commented Mar 27, 2024

View reviewed changes

Fix deprecation warning

1f02d83

jessepinho commented Mar 27, 2024

View reviewed changes

Fix test setup

e32f590

jessepinho changed the title ~~WIP: Update core dependencies~~ Update core dependencies Mar 27, 2024

jessepinho marked this pull request as ready for review March 27, 2024 23:03

jessepinho requested review from grod220, TalDerei, Valentine1898 and turbocrime March 27, 2024 23:03

Update to 0.70.3

3f07204

grod220 approved these changes Mar 28, 2024

View reviewed changes

grod220 reviewed Mar 28, 2024

View reviewed changes

conorsch changed the title ~~Update core dependencies~~ Update core dependencies to v0.70.3 Mar 28, 2024

TalDerei approved these changes Mar 28, 2024

View reviewed changes

jessepinho added 2 commits March 28, 2024 11:02

Add changeset

2f3f7ca

Throw when metadata is not found

f596006

jessepinho force-pushed the jessepinho/update-core branch from efb1341 to f596006 Compare March 28, 2024 18:24

jessepinho added 5 commits March 28, 2024 11:33

Rewrite getEpochByHeight as async iterator

87b6436

Refactor

b9fc0f4

Update proving key version

64a1525

Fix quote marks

cdfc04b

Fix linting issue

d581b4f

jessepinho force-pushed the jessepinho/update-core branch from 06d4390 to d581b4f Compare March 28, 2024 18:52

jessepinho merged commit 8933117 into main Mar 28, 2024
6 checks passed

jessepinho deleted the jessepinho/update-core branch March 28, 2024 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update core dependencies to v0.70.3 #839

Update core dependencies to v0.70.3 #839

jessepinho commented Mar 26, 2024 •

edited

Loading

TalDerei commented Mar 27, 2024

jessepinho Mar 27, 2024

jessepinho Mar 27, 2024

jessepinho Mar 27, 2024

jessepinho Mar 27, 2024

grod220 Mar 28, 2024

jessepinho Mar 27, 2024

jessepinho Mar 27, 2024

turbocrime Mar 28, 2024

jessepinho Mar 27, 2024

jessepinho Mar 27, 2024

grod220 Mar 28, 2024

jessepinho Mar 27, 2024

jessepinho Mar 27, 2024

grod220 left a comment

grod220 Mar 28, 2024

grod220 Mar 28, 2024

grod220 left a comment

TalDerei commented Mar 28, 2024

Update core dependencies to v0.70.3 #839

Update core dependencies to v0.70.3 #839

Conversation

jessepinho commented Mar 26, 2024 • edited Loading

In this PR

TalDerei commented Mar 27, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 left a comment

Choose a reason for hiding this comment

TalDerei commented Mar 28, 2024

jessepinho commented Mar 26, 2024 •

edited

Loading