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

pcli: add un-advertised support for importing Prax registry data #4801

Merged
merged 4 commits into from
Aug 12, 2024

Conversation

hdevalence
Copy link
Member

This makes a minimal set of changes to allow pcli to make use of the Prax registry. In order to not pre-empt more comprehensive planning about how such an integration should work, this PR just changes pcli so that if the registry is placed in a registry.json file in the pcli home directory, it will be imported into the view database on startup.

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    client-side changes only

This makes a minimal set of changes to allow `pcli` to make use of the Prax
registry. In order to not pre-empt more comprehensive planning about how such
an integration should work, this PR just changes `pcli` so that if the registry
is placed in a `registry.json` file in the `pcli` home directory, it will be
imported into the view database on startup.
Comment on lines 320 to +333
}
let mut selected_index = 0;
let mut selected_exponent = 0;
for (unit_index, unit) in self.inner.units.iter().enumerate() {
let unit_amount = Amount::from(10u128.pow(unit.exponent as u32));
if amount >= unit_amount {
return Unit {
unit_index,
inner: self.inner.clone(),
};
if unit_amount <= amount && unit.exponent >= selected_exponent {
selected_index = unit_index;
selected_exponent = unit.exponent;
}
}
self.base_unit()
return Unit {
unit_index: selected_index,
inner: self.inner.clone(),
};
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 bugfix needs a unit test.

It can be triggered by having a Metadata object where the list of units is not sorted. The previous code is incorrect on, for instance, the USDC Metadata currently in the Prax registry. That JSON object could be copied into a unit test, which can create a value like "1.234 USDC", parse it, and check that displaying it round trips (because the correct unit is selected).

@hdevalence
Copy link
Member Author

This doesn't fix problems with argument parsing, a lot of pcli arguments specify denoms and have the string parsing done inside clap, which has to be stateless. Full support will need to change this to take string args and then parse after the app is loaded and the registry can be pulled from the view database. But that can be future work

Copy link
Contributor

@aubrika aubrika left a comment

Choose a reason for hiding this comment

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

Confirmed to work locally when a properly formatted registry.json is present, LGTM

@aubrika aubrika merged commit 5ed9813 into main Aug 12, 2024
13 checks passed
@aubrika aubrika deleted the pcli-prax-registry branch August 12, 2024 17:41
conorsch pushed a commit that referenced this pull request Aug 12, 2024
This makes a minimal set of changes to allow `pcli` to make use of the
Prax registry. In order to not pre-empt more comprehensive planning
about how such an integration should work, this PR just changes `pcli`
so that if the registry is placed in a `registry.json` file in the
`pcli` home directory, it will be imported into the view database on
startup.

(cherry picked from commit 5ed9813)
conorsch pushed a commit that referenced this pull request Aug 12, 2024
This makes a minimal set of changes to allow `pcli` to make use of the
Prax registry. In order to not pre-empt more comprehensive planning
about how such an integration should work, this PR just changes `pcli`
so that if the registry is placed in a `registry.json` file in the
`pcli` home directory, it will be imported into the view database on
startup.

(cherry picked from commit 5ed9813)
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