-
Notifications
You must be signed in to change notification settings - Fork 93
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 codec borsh feature #273
Conversation
Codecov ReportBase: 67.46% // Head: 66.14% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
==========================================
- Coverage 67.46% 66.14% -1.32%
==========================================
Files 125 125
Lines 12796 13051 +255
==========================================
Hits 8633 8633
- Misses 4163 4418 +255
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. |
@DaviRain-Su is this ready to be reviewed? Whenever you're ready, please mark the PR as "ready for review" and I'll review it! |
yet, it can review |
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.
Once again, great work here.
This is just a preliminary review, will finish it later!
I notice that a subset of the types were tagged as serializable/deserializable by borsh and parity-scale-codec. What is the reasoning behind the choice of which type should be serializable/deserializable? |
In the substrate pallet we use the scale-codec encoding, while using borsh is the next contract we have to use |
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.
almost there 😃
- Add parity-scale-codec and Borsh below type - core::ics02_client::trust_threshold - core::ics03_connection::connection - core::ics03_connection::version - core::ics04_channel::channel - core::ics04_channel::commitment - core::ics04_channel::version - core::ics26_routing (ModuleId) - crate::Signer type
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.
Left a few improvements!
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.
A few uses of the derive macros were forgotten, and left a few nits!
5a79b1f
to
ddb0200
Compare
this is error, I change they suggest but have also error.
|
The CI fails because Rust 1.66 was just released. This fixes it: diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs
index 05b48096..6ffffae1 100644
--- a/crates/ibc/src/mock/context.rs
+++ b/crates/ibc/src/mock/context.rs
@@ -1879,7 +1879,7 @@ mod tests {
_relayer: &Signer,
) -> OnRecvPacketAck {
OnRecvPacketAck::Successful(
- Box::new(MockAck::default()),
+ Box::<MockAck>::default(),
Box::new(|module| {
let module = module.downcast_mut::<FooModule>().unwrap();
module.counter += 1; |
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.
Thank you @DaviRain-Su! I very much appreciate your contributions 😄
Closes: #259
Description
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.