-
Notifications
You must be signed in to change notification settings - Fork 983
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 transfer triggered vps #3804
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3804 +/- ##
==========================================
+ Coverage 72.78% 72.81% +0.02%
==========================================
Files 338 338
Lines 103923 104164 +241
==========================================
+ Hits 75637 75842 +205
- Misses 28286 28322 +36 ☔ View full report in Codecov by Sentry. |
06a80ae
to
02e9057
Compare
2f0db67
to
b634d91
Compare
crates/ibc/src/lib.rs
Outdated
{ | ||
self.verifiers.borrow_mut().insert( | ||
Address::from_str(verifier.as_ref()).map_err(|_| { | ||
Error::NftTransfer(NftTransferError::Other( |
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.
Does this only for NFT transfers? I'm confused about the choice of error 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.
You're right, could be both a fungible or non-fungible transfer. I've added a new error variant in 41583eb
Describe your changes
Addresses a bug discovered while attempting shielded actions on campfire. With #2928 we stopped triggering vps for the addresses that do not occupy the first segment of a storage keys. This has an effect on transfers, since the balance keys have as their first address the multitoken account. Our current implementation of the
transfer
andmulti-transfer
prelude functions manually triggers the vp of the source, but we do nothing for the destination(s), so we are not triggering their vps right now.This PR implements the followings:
transfer
andmulti-trasnfer
functions now also trigger the vps of the destinationsexecute
method of the ibc context triggers the vp of the involved address for all the packets we supportis_masp_key
andis_masp_transfer_key
to ensure correct validationMisc:
unwrap_or_default
in the pgf vp which was not fixed in Propagates error fromis_proposal_accepted
#3700unwrap_or_default
on errors to avoid the above issueChecklist before merging
breaking::
labels