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

Add get/set_prefixed_denom APIs + apply some improvements #504

Closed
wants to merge 13 commits into from

Conversation

Farhad-Shabani
Copy link
Member

@Farhad-Shabani Farhad-Shabani commented Mar 7, 2023

Closes: #502


Description

Following the review of the similar context on IBC-go and the revisiting codes that were related to the PrefixedDenom, I have also applied and included some tweaks in this PR as follows. Did so for the sake of time-saving, I hope you'll like them ;)

Added missing APIs

  • get_prefixed_denom
  • get_all_prefixed_denoms
  • set_prefixed_denom
  • is_account_blocked
  • set_port: we had get_port but wasn’t offering an interface for setting it

Some API adjustment

The is_send_enabled and is_receive_enabled methods were returning a boolean whose sole purpose was to propagate errors. This does not correspond with Rust's error-handling convention and against the Result type mean. It was suitable, as long as no specific error reporting was required

More consistent naming

  • Consolidate some namings in some places, we were using address and others account for the variable/method names that are related to AccountId type.
  • There were still places not adapted to our naming conventions

Refactor the return type of process_recv_packet_execute

I came up with an idea to simplify the return type to the Result<ModuleExtras, TokenTransferError>, to make it easier to understand and implement new changes.

Changes around hash function

Hash function moved to utils since it isn't called only by commitments. Furthermore, the return type of the hasher function has been changed to the [u8;32] type for a fix-sized and more solid type. The same proposal looks applicable for CommitmentRoot, CommitmentProofBytes, and CommitmentPrefix to improve security matters, IMO.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Mar 7, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.32.0 milestone Mar 7, 2023
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Patch coverage: 70.86% and project coverage change: -0.05 ⚠️

Comparison is base (d0d0683) 71.94% compared to head (e38cfa4) 71.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #504      +/-   ##
==========================================
- Coverage   71.94%   71.90%   -0.05%     
==========================================
  Files         126      127       +1     
  Lines       15771    15826      +55     
==========================================
+ Hits        11347    11380      +33     
- Misses       4424     4446      +22     
Impacted Files Coverage Δ
crates/ibc/src/applications/transfer/context.rs 49.05% <0.00%> (-2.01%) ⬇️
crates/ibc/src/applications/transfer/denom.rs 83.98% <ø> (ø)
crates/ibc/src/applications/transfer/error.rs 0.00% <ø> (ø)
crates/ibc/src/applications/transfer/events.rs 8.51% <0.00%> (+0.17%) ⬆️
crates/ibc/src/applications/transfer/relay.rs 0.00% <0.00%> (ø)
.../src/applications/transfer/relay/on_recv_packet.rs 0.00% <0.00%> (ø)
crates/ibc/src/core/ics04_channel/packet.rs 47.30% <40.00%> (ø)
crates/ibc/src/mock/context.rs 75.30% <59.15%> (-0.49%) ⬇️
...tes/ibc/src/applications/transfer/msgs/transfer.rs 44.89% <66.66%> (-1.64%) ⬇️
.../src/core/ics04_channel/handler/acknowledgement.rs 85.51% <70.00%> (+0.27%) ⬆️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Farhad-Shabani Farhad-Shabani marked this pull request as ready for review March 7, 2023 18:27
@Farhad-Shabani Farhad-Shabani requested a review from plafer March 7, 2023 18:31
@Farhad-Shabani
Copy link
Member Author

The above matters have been opened as issues (#506, #507, #508, #509) to be included in the changelog.
we could even split the PRs to make things smoother, but I am concerned about merge conflicts. The places they touch are mostly related.

@Farhad-Shabani
Copy link
Member Author

Closed in favor of #512, #514, #516

@Farhad-Shabani Farhad-Shabani deleted the farhad/denom-trace branch March 30, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: breaking Admin: breaking change that may impact operators
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Enable token transfer app to store a map of trace hash with the associated denom
1 participant