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

fix: essence tranche token metadata #1798

Merged
merged 4 commits into from
Apr 15, 2024
Merged

fix: essence tranche token metadata #1798

merged 4 commits into from
Apr 15, 2024

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Apr 5, 2024

Description

Slack thread

We can hold this until #1756 is merged

Changes and Descriptions

  • Fixes incorrect tranche token metadata reported by pool system essence which is used for Updated event
  • Adds missing unit test which asserts condition

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@wischli wischli self-assigned this Apr 5, 2024
@wischli wischli added the I2-bug The code fails to follow expected behaviour. label Apr 5, 2024
@wischli wischli added this to the Centrifuge 1029 milestone Apr 5, 2024
lemunozm
lemunozm previously approved these changes Apr 5, 2024
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this fix @wischli and add tests related to it 💯

Just one NIT but LGTM!

tranche.currency.into(),
),
)
.ok_or(DispatchError::CannotLookup)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning that weird ok_or and removing the unwrap()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that code path gave me shivers

Comment on lines +2451 to +2454
token_name: BoundedVec::try_from("ResName".as_bytes().to_owned())
.expect("String not out of bounds"),
token_symbol: BoundedVec::try_from("ResSym".as_bytes().to_owned())
.expect("String not out of bounds"),
Copy link
Contributor

@lemunozm lemunozm Apr 5, 2024

Choose a reason for hiding this comment

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

NIT. If the test does not check token_name or token_symbol you can avoid initializing them using directly BoundedVec::default() to ease the reading of the test (same below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are checking the tranche metadata in L2498 which is why I did not want to use default values here

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, I didn't see it. Then everything is perfect!

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 48.45%. Comparing base (c63c370) to head (0bb1ea7).

Files Patch % Lines
pallets/pool-system/src/pool_types.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1798   +/-   ##
=======================================
  Coverage   48.45%   48.45%           
=======================================
  Files         168      168           
  Lines       13331    13332    +1     
=======================================
+ Hits         6459     6460    +1     
  Misses       6872     6872           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mustermeiszer
mustermeiszer previously approved these changes Apr 10, 2024
@wischli wischli dismissed stale reviews from mustermeiszer and lemunozm via 5903a10 April 15, 2024 12:43
@wischli wischli requested a review from lemunozm April 15, 2024 13:52
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

Reapproving!

@wischli wischli merged commit 58045f4 into main Apr 15, 2024
12 checks passed
@wischli wischli deleted the fix/pool-systems-essence branch April 15, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The code fails to follow expected behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants