-
Notifications
You must be signed in to change notification settings - Fork 956
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
Now hash the TransferTarget into Transaction transparent outputs. #3320
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3320 +/- ##
==========================================
- Coverage 53.89% 53.74% -0.15%
==========================================
Files 314 314
Lines 105704 105919 +215
==========================================
- Hits 56968 56931 -37
- Misses 48736 48988 +252 ☔ View full report in Codecov by Sentry. |
6de0599
to
dddc8aa
Compare
dddc8aa
to
20d113b
Compare
098ffe6
to
c5c615c
Compare
c5c615c
to
949faaa
Compare
&msg.timeout_height_on_b, | ||
&msg.timeout_timestamp_on_b, | ||
); | ||
if packet_commitment != storage_commitment { |
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.
The packet commitment from the packet has been validated in the IBC VP.
We need to check the receiver in the IBC message with that of the MASP transaction, right?
I think we make a packet commitment from the MASP transaction's receiver and the IBC message(tx_data)'s timeout values and compare it with the commitment stored in the storage.
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.
Understood. I'm working on a variant of this PR where the timeout values are extracted from tx_data. I'm just a little concerned that this will cause the VP to be dependent on the transaction format instead of the state changes. Also, I haven't yet checked how this will interact with the removal of Transfer
dependence changes in #3232 .
changed_balances.decoder.insert(ibc_address_hash, ibc_address); | ||
|
||
// Extract the IBC events | ||
let ibc_events: BTreeSet<_> = self |
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.
We don't need IBC events because the transaction has only one IBC message.
We can get the IBC transfer info by decoding the tx_data like
Line 156 in 879a326
let message = decode_message(tx_data)?; |
949faaa
to
f6a7580
Compare
e713123
to
e3ce9a5
Compare
e3ce9a5
to
6ad0a0f
Compare
6ad0a0f
to
abbf3b2
Compare
f77cf47
to
5547f09
Compare
Describe your changes
An attempt to address issue #3300. The approach taken is to include the IBC receiver in the pre-image of shielded
Transaction
outputs. More specifically, the following changes have been made:Address
indicating a non-IBC recipientAddress
that corresponds to an transferPacketData
and checking that the transparent inputs and outputs use thatTransaction
go to thereceiver
stated inPacketData
in the case of an outgoing shielded actionTransaction
are the IBC internal address in the case of a refund or an incoming shielded actionCloses #3300
Indicate on which release or other PRs this topic is based on
Namada v0.37.0
Hermes 1.8.2 using this branch for the Namada SDK
Checklist before merging to
draft