Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bech32m #979

Merged
merged 1 commit into from
Apr 26, 2024
Merged

bech32m #979

merged 1 commit into from
Apr 26, 2024

Conversation

turbocrime
Copy link
Contributor

@turbocrime turbocrime commented Apr 24, 2024

fixing #778 and providing typesafe, named functions

also updating most of the tests to use more realistic data, now that lengths and checksums are validated everywhere.

this package is configured to build with tsc for publish, despite discussion of not building - it's a relatively simple package, the main export is all constant primitives, and it may be used in simpler contexts than the larger and more complex packages that depend on message types.

the diff is huge, because these functions are used absolutely everywhere.

@turbocrime turbocrime marked this pull request as ready for review April 24, 2024 23:29
Copy link
Collaborator

@grod220 grod220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests!

packages/bech32m/src/penumbra.ts Outdated Show resolved Hide resolved
packages/bech32m/src/penumbra.ts Outdated Show resolved Hide resolved
packages/bech32m/src/format/convert.ts Outdated Show resolved Hide resolved
packages/bech32m/src/format/convert.ts Show resolved Hide resolved
apps/minifront/src/state/staking/index.test.ts Outdated Show resolved Hide resolved
packages/bech32m/src/format/prefix.ts Outdated Show resolved Hide resolved
packages/bech32m/src/format/prefix.ts Outdated Show resolved Hide resolved
packages/bech32m/src/format/size.ts Outdated Show resolved Hide resolved
packages/bech32m/src/index.ts Show resolved Hide resolved
packages/ui/components/ui/identity-key-component.tsx Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests for removed function deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use replaced with getValidatorIdentityKey plus bech32IdentityKey

@turbocrime turbocrime requested a review from grod220 April 25, 2024 21:07
@turbocrime turbocrime force-pushed the turbocrime/bech32m branch 3 times, most recently from 1e2133a to e378ed5 Compare April 25, 2024 21:18
};
}>;

export default {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid using default exports in general

@@ -0,0 +1,30 @@
import SPEC from './format';

export default SPEC;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, let's name our exports

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package seems to have multiple ways its accessing the same information

A)
import { Prefixes } from './format/prefix';
const prefix = Prefixes.passet;

B)
import SPEC from './format';
export const PENUMBRA_BECH32M_ASSETID_PREFIX = SPEC.passet.prefix;

Instead of defining all of these parameters in separate objects, it feels simpler to just put them all in this file. Aka, this:

export const bech32Spec = {
  passet: {
    prefix: 'passet',
    stringLength: 65,
    byteLength: 32,
    innerName: "inner",
  },
}

versus

import { Inner } from './inner';
import { StringLength } from './strings';
import { ByteLength } from './bytes';
import { Prefixes } from './prefix';

export const bech32Spec = {
  passet: {
    prefix: Prefixes.passet,
    stringLength: StringLength.passet,
    byteLength: ByteLength.passet,
    innerName: Inner.passet,

This will allow us also to delete packages/bech32m/src/index.ts entirely

Comment on lines +11 to +13
export const assetIdFromBech32m = (passet1: string): { [innerName]: Uint8Array } => ({
[innerName]: fromBech32m(passet1 as `${typeof prefix}1${string}`, prefix),
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole purpose of this package is to allow us easy ways to convert to/from bech32 to/from Penumbra protobuf types. Returning approximations of the inside of these types feels unnecessary, especially given all consumers will then be required to construct the type manually right after. The more natural API is to simply return what they need. I think we should do this:

- export const assetIdFromBech32m = (passet1: string): { [innerName]: Uint8Array }
+ export const assetIdFromBech32m = (passet1: string): AssetId

Copy link
Contributor Author

@turbocrime turbocrime Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole purpose of the package is to verify bech32m or convert from bech32m to binary data in a verifiable way.

the types are a convenience. i would prefer to remove the structure than to add the types.

Copy link
Collaborator

@grod220 grod220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked offline. Can revisit these topics later.

@turbocrime turbocrime merged commit fdd4303 into main Apr 26, 2024
1 check passed
@turbocrime turbocrime deleted the turbocrime/bech32m branch April 26, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants