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

Rename token to assetHash for spot assets #263

Merged
merged 6 commits into from Feb 13, 2023
Merged

Conversation

adamiak
Copy link
Member

@adamiak adamiak commented Feb 12, 2023

Resolves L2B-824

Up until now we have used the name token and assumed it's a PedersenHash when dealing with spot assets (for example in VaultLeaf and vaults table). After recent refactoring we have a proper AssetHash type for spot assets. We also know it's not a Pedersen hash but a sliced Keccak.

This PR renames token to assetHash and the type from PedersenHash to AssetHash. The name token is preserved only in zed parsing code as this is the name that comes from DAC.

@Wendrowiec13
Copy link
Contributor

Wendrowiec13 commented Feb 13, 2023

Screenshot 2023-02-12 at 20 52 08

Shouldn't `PedershenHash0x` here be renamed to AssetHash?

Comment on lines +10 to +11
if (value.startsWith('0x')) {
value = value.slice(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is a better approach. We are slicing the 0x prefix if it's there and then we are adding it anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is done to pad the length to 64. So that 0x000ab and 0xab end up being equal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense then

@linear
Copy link

linear bot commented Feb 13, 2023

L2B-824 Use AssetHash type for Spot assets, unify naming

Perpetual: - assetId

Spot: - assetHash

@adamiak
Copy link
Member Author

adamiak commented Feb 13, 2023

Shouldn't PedershenHash0x here be renamed to AssetHash?

@Wendrowiec13 good spot, fixed.

@adamiak adamiak merged commit 1ac282f into master Feb 13, 2023
@adamiak adamiak deleted the rename-spottoken-assethash branch February 13, 2023 16:43
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.

None yet

3 participants