-
Notifications
You must be signed in to change notification settings - Fork 17
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
Use new registry [part 3] - assets registry #952
Conversation
e78a11f
to
2ff328d
Compare
apps/extension/package.json
Outdated
@@ -20,7 +20,7 @@ | |||
"@bufbuild/protobuf": "^1.8.0", | |||
"@connectrpc/connect": "^1.4.0", | |||
"@connectrpc/connect-web": "^1.4.0", | |||
"@penumbra-labs/registry": "4.0.0", | |||
"@penumbra-labs/registry": "^4.1.0", |
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.
Noticed that this package is published by the penumbra-labs organization, but is in organization's prax-wallet repository. I'm assuming this is intentional
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.
Why this change to add the ^
? Just general best practice?
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.
Was mistakenly pinned before. The caret allows for minor/patch updates to be downloaded as well.
const getIbcConnections = async () => { | ||
const chainId = await getChainId(); | ||
if (!chainId) throw new Error('Could not fetch chain id'); | ||
|
||
const registryClient = new ChainRegistryClient(); | ||
const { ibcConnections } = await registryClient.get(chainId); | ||
return ibcConnections; | ||
}; | ||
|
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.
interaction with registry moved to fetchers/registry.ts
numeraires: Metadata[]; | ||
stakingTokenMetadata: Metadata; |
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.
numeraires
and stakingTokenMetadata
are passed at initialization to avoid registry dependency for the block processor
equivalentValues: [ | ||
{ | ||
equivalentAmount: { hi: 0n, lo: 0n }, | ||
numeraire: STAKING_TOKEN_METADATA, | ||
}, | ||
], |
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.
I could add a registry dependency for view-service
, but I don't see much point in doing that just for the sake of adding zero equivalentValues
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.
Hmm... this means, though, that the staking page will not show the equivalent value of 0 UM
for delegation tokens that the user doesn't hold, which is needed as part of the staking page design. If we're pulling metadata from IndexedDB, we don't need to hit the registry — we could just query for the staking token metadata in our database.
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.
In order to get metadata from indexed-db I need to know which metadata corresponds to the staking token. And at the moment, I can only get the assetId of staking token from registry. And this leads to the fact that I have to access the registry at some point.
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.
Ahh, true. Hm. We do know that the display
will always be penumbra
though, right? So we could look for the metadata that has that property?
Or, we could hard-code the UM asset ID in code, since that should also never change, right?
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.
Yes, it is.
But at the same time, Gabe suggested adding STAKING_TOKEN
to the registry and now, it seems to me, we should consider the registry the only reliable source about STAKING_TOKEN
(actually, now I think this is quite redundant, and this PR would be much less if we had left STAKING_TOKEN just a constant)
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.
we should provide assetid of staking token to view service as a HandlerContext item
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.
we should provide assetid of staking token to view service as a HandlerContext item
Do I understand correctly that in this case we will pass the assetid of staking token as context key for all rpc calls, although it is used only in one?
I have my doubts about this way in general, as we may end up with a lot of context keys, which will always be passed but rarely used
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.
In order to pass stakingTokenId as a context key you need to do a lot of work.
The thing is that service-worker cannot access registry because it has no chain-id, we get chain-id when initializing services through querier, and for that querier must be initialized.
I don't think it shouldn't be done, but I suggest to do it in a separate PR on HanlderContext refactoring
import type { IdbUpdate, PenumbraDb } from '@penumbra-zone/types/src/indexed-db'; | ||
|
||
describe('IndexedDb', () => { | ||
// uses different wallet ids so no collisions take place | ||
const generateInitialProps = () => ({ | ||
chainId: 'test', | ||
accountAddr: 'penumbra123xyz', |
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.
deprecated property
<div className='font-mono'> | ||
{joinLoHiAmount(getAmount(claimFee)).toString()} upenumbra | ||
</div> |
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.
const getClaimFeeValueView = (claimFee: Fee) => | ||
new ValueView({ | ||
valueView: { | ||
case: 'knownAssetId', | ||
value: { | ||
amount: claimFee.amount, | ||
metadata: STAKING_TOKEN_METADATA, | ||
}, | ||
}, | ||
}); |
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.
I believe that the ui package should not depend on registry
. And also we should not pass additional data for rendering transaction view components
In this case I had to simplify the fee display and get rid of ValueView
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.
Hmm, yeah, this is tricky. Can't think of a better solution for this either.
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.
it looks like there is no metadata field on the Fee type. we are using tools from @penumbra-zone/perspective
to prepare the view prop everywhere we use this component, including a metadata getter. so i think options are
- pr core for a metadata field on Fee, and prepare it before using component, as now
- add a function prop to get metadata, component now responsible for all metadata
- add a static optional prop for staking token metadata, component nearly identical
i prefer 3 because staking token metadata is a chain-level special case, and it's minimal change. the current behavior in this pr is good and should remain as fallback if prop is undefined/absent.
this can be a separate issue not addressed in this pr.
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.
Yeah, definitely doesn't need to be addressed as part of this PR
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.
Re: point 1: I actually suspect that a better solution would be to represent the fee as a ValueView
generated from the Fee
, since Fee
already contains both an amount and an asset ID. So e.g., in addition to the swapPlaintext.claimFee
field, there could also be a swapPlaintext.claimFeeView
field that's a ValueView
. Thoughts?
(Again, though, out of scope for this PR.)
8a61535
to
cfcd1eb
Compare
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.
OK, I left a lot of comments, but almost all of them are about one thing: we should (IMO) populate our IndexedDB with asset metadata from the registry every time we detect a new asset type in the block processor. Then, instead of calling to GitHub in multiple places, we should call to the extension/view service to get the metadata we need.
Thoughts?
* A default `ValueView` to render when we don't have any balance data for a | ||
* particular token. | ||
*/ | ||
export const zeroValueView = (metadata: Metadata) => |
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.
This shouldn't really be in the components
directory, but rather somewhere like apps/minifront/src/utils
apps/extension/package.json
Outdated
@@ -20,7 +20,7 @@ | |||
"@bufbuild/protobuf": "^1.8.0", | |||
"@connectrpc/connect": "^1.4.0", | |||
"@connectrpc/connect-web": "^1.4.0", | |||
"@penumbra-labs/registry": "4.0.0", | |||
"@penumbra-labs/registry": "^4.1.0", |
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.
Why this change to add the ^
? Just general best practice?
@@ -89,10 +91,12 @@ export const unclaimedSwapsWithMetadata = async (): Promise<UnclaimedSwapsWithMe | |||
export const SwapLoader: LoaderFunction = async (): Promise<SwapLoaderResponse> => { | |||
await throwIfPraxNotConnectedTimeout(); | |||
|
|||
const registryAssets = await getAssetsFromRegistry(); |
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.
Hm — this means we'll be hitting GitHub every time we load the swap page, which is a privacy concern. Shouldn't we pull from the asset registry in the block processor (e.g., in identifyNewAssets
), save them to the database there, and then get the assets from the extension here?
My understanding is that the extension should be the backend for minifront. Minifront shouldn't talk to any other services directly.
apps/minifront/src/components/staking/account/delegation-value-view/index.tsx
Outdated
Show resolved
Hide resolved
set(state => { | ||
state.staking.unstakedTokensByAccount = unstakedTokensByAccount; | ||
state.staking.accountSwitcherFilter = accountSwitcherFilter; | ||
}); | ||
}, | ||
delegate: async () => { | ||
try { | ||
const req = assembleDelegateRequest(get().staking); | ||
const stakingTokenMetadata = await getStakingTokenMetadata(); |
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.
should pull from extension instead of GitHub
@@ -99,10 +101,14 @@ export class Services implements ServicesInterface { | |||
sctParams: { epochDuration }, | |||
} = params; | |||
|
|||
const registryClient = new ChainRegistryClient(); | |||
const { stakingAssetId, assetById, numeraires } = await registryClient.get(chainId); |
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.
Grab from IndexedDB if possible
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.
I'm not sure I'm understanding your idea correctly.
The thing is that I get the assetById
here just to pass to indexed-db for initialization.
On initialization indexed-db will pre-populate the asset table with these values.
And yes, this will happen every time indexed-db is initialized.
Regarding numeraires
and stakingAssetId
, I can create a table for them in indexed-db, but to populate that table with data I still have to go to registry, I don't really understand what problem this solves, at the same time we have to think about the fact that numeraires
or stakingAssetId
in indexed-db might be considered outdated
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.
I might be misunderstanding something here, but let me explain what I'm trying to suggest and see if it sounds right to you.
So, at the moment, we generally avoid making calls to an RPC node (or any other server, like GitHub) whenever possible — except for in the block processor. In the block processor, we make calls to the RPC node whenever we need to, since that doesn't reveal anything about the user's intentions. Everyone who uses the Prax extension uses the block processor and thus makes the exact same RPC/API calls.
But if we use the ChainRegistryClient
in both minifront and in the extension's RPC implementations, those calls will fire on an as-needed basis, which may leak information about the user's intentions.
Thus, my argument is that we should fetch data from the registry in the block processor here. That way, we're guaranteed to have an up-to-date set of asset metadata at any given time, since we'll always know about every asset type that's been traded/etc. on chain.
Let me know if my understanding is correct — perhaps my thinking doesn't apply to IBC, since we'll need to know about tokens that can be traded in/out of Penumbra, even if they haven't been yet?
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.
That way, we're guaranteed to have an up-to-date set of asset metadata at any given time, since we'll always know about every asset type that's been traded/etc. on chain.
the saveAndReturnMetadata()
function is called only for those assets that occur on the user's balance. This does not exactly guarantee that we will have a local copy of the metadata of all assets in the indexed-db
I can see some disadvantages to using a local copy of the asset registry
- we do not know when the local copy was last updated and whether it contains an up-to-date list of assets
- In addition to saving, we will probably have to make a mechanism for updating the metadata and deleting the metadata. (I've presented a case where the registry initially had the wrong metadata but was corrected afterwards, but the local copies will never be changed).
Also, it seems to me that there is no privacy issue in getting data from github because in fact we are reading all datadata from the registry (ibc, rpc, all assets metadata) on every call (one json file is being read) and it literally does not reveal any of our intentions.
Also, as far as I understand, Gabe has created a mechanism to cache the registry, meaning that the data will be read only once, and the next calls will use the cache.
And considering that we already use the registry to get ibc channel data, nothing changes from the point of view of the github worker who is watching us
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.
chainId
should not change often, and if it does our state is invalid - we query app parameters in order to verify it has not changed. i believe stakingTokenId
should not change unless chainId
changes.
there is no explicit stakingTokenId
in app parameters, but i believe staking tokens should be present at the genesis block, so could detect it without using the registry client.
staking token id is generated in core repo from known values https://github.com/penumbra-zone/penumbra/blob/main/crates/core/asset/src/asset/cache.rs#L28
we may be able to expose this via wasm. https://github.com/penumbra-zone/penumbra/blob/main/crates/core/asset/src/asset/id.rs#L156
notably LP uses the same hash and technique, which is exposed via wasm https://github.com/penumbra-zone/penumbra//blob/main/crates/core/component/dex/src/lp/position.rs#L103
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.
we may be able to expose this via wasm
Yes, but it doesn't improve anything because we would still need to get the metadata (from the registry or indexed-db) and we would still need to call the registry for other things (assets, numeraires)
Also, we probably don't want minifront to use wasm, and we need stakingTokenId there too
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.
OK, gotcha. Well, my instinct is to say that it probably isn't a big concern if these requests are being sent to GitHub, since the RPC node has no access to GitHub's internal server access logs. But I'm still a little wary of approving it — this is probably a question for @grod220 since it's sort of a product issue.
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.
So, at the moment, we generally avoid making calls to an RPC node (or any other server, like GitHub) whenever possible — except for in the block processor.
I think the principle is something like:
- Do not make a request to node when we can locally serve the result from indexeddb
- If making requests to node, do our best to ensure it doesn't leak privacy (aka query node for whole block instead of specific tx’s).
- If there’s no way to obfuscate, make it opt in (e.g. swap estimations are triggered via button with a privacy notice)
In this case, querying the registry at initialization does not reveal any specific intentions besides wanting to use the system generally. I don’t see an issue with this.
However, as has been suggested in the review, minifront should be requesting asset denom data via rpc calls and not from github directly.
the saveAndReturnMetadata() function is called only for those assets that occur on the user's balance. This does not exactly guarantee that we will have a local copy of the metadata of all assets in the indexed-db
I agree, we should be able to utilize the registry to pre-populate at initialization and the block processor to add to the assets table if it discovers new ones.
I can see some disadvantages to using a local copy of the asset registry
- we do not know when the local copy was last updated and whether it contains an up-to-date list of assets
Given this happens at service worker initialization, indexeddb should pretty much always have the latest from the registry.
In addition to saving, we will probably have to make a mechanism for updating the metadata and deleting the metadata.
Saving just overwrites the old, so this is probably fine. Not sure the use-case for deleting, but can revisit that later.
equivalentValues: [ | ||
{ | ||
equivalentAmount: { hi: 0n, lo: 0n }, | ||
numeraire: STAKING_TOKEN_METADATA, | ||
}, | ||
], |
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.
Hmm... this means, though, that the staking page will not show the equivalent value of 0 UM
for delegation tokens that the user doesn't hold, which is needed as part of the staking page design. If we're pulling metadata from IndexedDB, we don't need to hit the registry — we could just query for the staking token metadata in our database.
const getClaimFeeValueView = (claimFee: Fee) => | ||
new ValueView({ | ||
valueView: { | ||
case: 'knownAssetId', | ||
value: { | ||
amount: claimFee.amount, | ||
metadata: STAKING_TOKEN_METADATA, | ||
}, | ||
}, | ||
}); |
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.
Hmm, yeah, this is tricky. Can't think of a better solution for this either.
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.
Changing my review to "Comment" so it's not blocking if someone else approves
apps/extension/package.json
Outdated
@@ -20,7 +20,7 @@ | |||
"@bufbuild/protobuf": "^1.8.0", | |||
"@connectrpc/connect": "^1.4.0", | |||
"@connectrpc/connect-web": "^1.4.0", | |||
"@penumbra-labs/registry": "4.0.0", | |||
"@penumbra-labs/registry": "^4.1.0", |
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.
Was mistakenly pinned before. The caret allows for minor/patch updates to be downloaded as well.
@@ -99,10 +101,14 @@ export class Services implements ServicesInterface { | |||
sctParams: { epochDuration }, | |||
} = params; | |||
|
|||
const registryClient = new ChainRegistryClient(); | |||
const { stakingAssetId, assetById, numeraires } = await registryClient.get(chainId); |
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.
So, at the moment, we generally avoid making calls to an RPC node (or any other server, like GitHub) whenever possible — except for in the block processor.
I think the principle is something like:
- Do not make a request to node when we can locally serve the result from indexeddb
- If making requests to node, do our best to ensure it doesn't leak privacy (aka query node for whole block instead of specific tx’s).
- If there’s no way to obfuscate, make it opt in (e.g. swap estimations are triggered via button with a privacy notice)
In this case, querying the registry at initialization does not reveal any specific intentions besides wanting to use the system generally. I don’t see an issue with this.
However, as has been suggested in the review, minifront should be requesting asset denom data via rpc calls and not from github directly.
the saveAndReturnMetadata() function is called only for those assets that occur on the user's balance. This does not exactly guarantee that we will have a local copy of the metadata of all assets in the indexed-db
I agree, we should be able to utilize the registry to pre-populate at initialization and the block processor to add to the assets table if it discovers new ones.
I can see some disadvantages to using a local copy of the asset registry
- we do not know when the local copy was last updated and whether it contains an up-to-date list of assets
Given this happens at service worker initialization, indexeddb should pretty much always have the latest from the registry.
In addition to saving, we will probably have to make a mechanism for updating the metadata and deleting the metadata.
Saving just overwrites the old, so this is probably fine. Not sure the use-case for deleting, but can revisit that later.
stakingTokenMetadata: Metadata.fromJson(assetById[stakingAssetId]!), | ||
numeraires: numeraires.map(numeraires => Metadata.fromJson(assetById[numeraires]!)), |
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.
Ah, our registryClient
should be deserializing so consumers don't have to do this. I'll open a PR on the other repo to get your thoughts.
0d425e9
to
31709f4
Compare
export const getAllAssets = async (): Promise<Metadata[]> => { | ||
const responses = await Array.fromAsync(viewClient.assets({})); | ||
return responses.map(getDenomMetadata); | ||
}; |
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.
now we get assets from view service instead of registry
export const getStakingTokenMetadata = async () => { | ||
const chainId = await getChainId(); | ||
if (!chainId) { | ||
throw new Error('Could not fetch chain id'); | ||
} | ||
|
||
const registryClient = new ChainRegistryClient(); | ||
const { stakingAssetId } = await registryClient.get(chainId); | ||
const stakingAssetsMetadata = await getAssetMetadataById(stakingAssetId); | ||
|
||
if (!stakingAssetsMetadata) { | ||
throw new Error('Could not fetch staking asset metadata'); | ||
} | ||
return stakingAssetsMetadata; | ||
}; |
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.
now we get the stakingAssetId from the registry, and the metadata from the view service. (this makes no sense, but we should always favor local data for privacy reasons)
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.
Gotcha — perhaps we can add stakingAssetId
to AppParameters
in the future.
* A default `ValueView` to render when we don't have any balance data for a | ||
* particular token. | ||
*/ | ||
export const zeroValueView = (metadata: Metadata) => |
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.
moved by Jesse suggestion
equivalentValues: [ | ||
{ | ||
equivalentAmount: { hi: 0n, lo: 0n }, | ||
numeraire: STAKING_TOKEN_METADATA, | ||
}, | ||
], |
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.
In order to pass stakingTokenId as a context key you need to do a lot of work.
The thing is that service-worker cannot access registry because it has no chain-id, we get chain-id when initializing services through querier, and for that querier must be initialized.
I don't think it shouldn't be done, but I suggest to do it in a separate PR on HanlderContext refactoring
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.
Great job! And thanks for all the inline comments which helped immensely with reviewing.
export const getStakingTokenMetadata = async () => { | ||
const chainId = await getChainId(); | ||
if (!chainId) { | ||
throw new Error('Could not fetch chain id'); | ||
} | ||
|
||
const registryClient = new ChainRegistryClient(); | ||
const { stakingAssetId } = await registryClient.get(chainId); | ||
const stakingAssetsMetadata = await getAssetMetadataById(stakingAssetId); | ||
|
||
if (!stakingAssetsMetadata) { | ||
throw new Error('Could not fetch staking asset metadata'); | ||
} | ||
return stakingAssetsMetadata; | ||
}; |
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.
Gotcha — perhaps we can add stakingAssetId
to AppParameters
in the future.
Co-authored-by: Jesse Pinho <jesse@jessepinho.com>
…-view/index.tsx Co-authored-by: Jesse Pinho <jesse@jessepinho.com>
Co-authored-by: Jesse Pinho <jesse@jessepinho.com>
31709f4
to
0d6eca9
Compare
No description provided.