-
Notifications
You must be signed in to change notification settings - Fork 208
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 new Vtransfer module for notifications #8624
Conversation
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.
Looking good! Here're some questions and suggestions.
e06b26c
to
8e15046
Compare
af2951e
to
8faa03a
Compare
b18ae03
to
bb4436e
Compare
481eb54
to
bb4436e
Compare
45d7718
to
9b9ab3a
Compare
10a5707
to
555df1f
Compare
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.
I think we need to change the intercept
name. I'd also like to see the types, though that's easier to amend later.
bcd287a
to
7f3be7d
Compare
…try rather than in middleware This prevents the authority leak described at Agoric#8624 (comment) .
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.
🎉
#8624 (comment) makeBridgeTargetKit can be more general than "IBC transfer", and is already pulled in separately by vat-transfer.js
…9471) #8624 (comment) makeBridgeTargetKit can be more general than "IBC transfer", and is already pulled in separately by vat-transfer.js
panic(fmt.Errorf("SwingStore export dir not set")) | ||
am.swingStoreExportDir = "/tmp/swingset_export" |
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.
What is the motivation for this change? This is used by genesis import/export which should set this appropriately.
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.
I don't know the motivation, but I see that @schnetzlerjoe made the change. Perhaps this panic was being triggered by the x/vtransfer ibc_middleware_test.go
?
closes: #8583, closes: #9059, closes: #9256
Description
This PR adds the vTransfer middleware module which acts as a notification module on the transfer port for contracts that need to act based on it.
Security Considerations
As discussed, this does change how inbound assets are handled.
Testing Considerations
The middleware module has mock unit tests in ibc_middleware_test.go