-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add badges to registry #106
Conversation
|
pub images: Vec<Image>, | ||
} | ||
|
||
#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
pub struct Image { | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub png: Option<String>, | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub svg: Option<String>, | ||
} | ||
|
||
impl From<Image> for AssetImage { | ||
fn from(image: Image) -> Self { | ||
AssetImage { | ||
png: image.png.unwrap_or_default(), | ||
svg: image.svg.unwrap_or_default(), | ||
theme: None, | ||
} | ||
} | ||
} | ||
|
||
pub trait IntoPbImages { | ||
fn into_pb_images(self) -> Vec<AssetImage>; | ||
} | ||
|
||
impl IntoPbImages for Vec<Image> { | ||
fn into_pb_images(self) -> Vec<AssetImage> { | ||
self.into_iter().map(AssetImage::from).collect() | ||
} |
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.
Found defining a local image type to be unnecessary when we can just import the protobuf type directly
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 chain input had an overlapping id with another input json. This means it did not render. Can delete.
npm/src/remote.test.ts
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.
New api on the fetch-mock library w/ version bump
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.
Labels all the "native" alloy assets coming from osmosis with a chain badge
input/chains/penumbra-1.json
Outdated
"badgesByBase": { | ||
"transfer/channel-4/factory/osmo1em6xs47hd82806f5cxgyufguxrrc7l0aqx7nzzptjuqgswczk8csavdxek/alloyed/allUSDT": [ | ||
{ | ||
"png": "https://raw.githubusercontent.com/cosmos/chain-registry/master/osmosis/images/osmo.png", | ||
"svg": "https://raw.githubusercontent.com/cosmos/chain-registry/master/osmosis/images/osmo.svg" | ||
} | ||
], |
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.
what if we defined a single badge entry for assets sharing the same badge?
"badges": {
"osmosis": {
"png": "image link",
"svg": "image link"
}
},
"badgesByBase": {
"transfer/channel-4/...": ["osmosis"],
// other channels
}
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.
Good suggestion, added
.badges_by_base | ||
.get(&metadata.base_denom().denom) | ||
{ | ||
let mut pb_metadata: pb::Metadata = metadata.clone().into(); |
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.
cloning the metadata necessary here?
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.
Can't do an into() from a mutable metadata domain type
expect(res.badges.length).toEqual(1); | ||
expect(res.badges[0]?.png).toEqual( | ||
'https://raw.githubusercontent.com/prax-wallet/registry/main/images/penumbra-favicon.png', | ||
); |
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.
test coverage can be expanded – assets without badges and missing 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.
Missing metadata test already there. Added test for getting asset without badge.
Closes: #97
Related: penumbra-zone/web#1864
Adds the badge for the osmosis chain icon to all alloyed assets coming from Osmosis.