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

refactor(vats): Remove makeBridgeTargetKit from prepareTransferTools #4

Conversation

gibson042
Copy link

Agoric#8624 (comment)

makeBridgeTargetKit can be more general than "IBC transfer", and is already pulled in separately by vat-transfer.js

mergify bot and others added 23 commits June 8, 2024 04:04
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
closes: #XXXX
refs: #9465 

## Description

This PR just captures the test from #9465 , turning it into an expected
failure. A later PR that fixes the bug would then remove the `.failing`.

### Security Considerations

The bug itself endangers integrity, so fixing it will help security.
This PR though just adds the test that demonstrates the bug.

### Scaling Considerations

none

### Documentation Considerations

none
### Testing Considerations

The point. 
### Upgrade Considerations

The bug means that an asyncFlow activation that does return early would
be committed to the done state, preventing a later upgrade from
repairing it to get beyond the problem. But this PR just adds the test
for that bug.
more accurate. What's built is a code/permit pair (and its bundles), not a proposal (which has a title and a list of core evals)
## Description

I was recently confused for the nth time on why writeCoreProposal wasn't
writing a proposal. Now that we model proposals in `a3p-integration`
(something proposed to stakers) I've been running into this confusion
more often.


[CoreEvalProposal](https://github.com/Agoric/agoric-sdk/blob/4491bd134d194ba5a965a6b0ff9f394563581162/golang/cosmos/x/swingset/types/swingset.pb.go#L28-L37)
has a title and `evals` which isis an array of
[CoreEval](https://github.com/Agoric/agoric-sdk/blob/4491bd134d194ba5a965a6b0ff9f394563581162/golang/cosmos/x/swingset/types/swingset.pb.go#L72-L81).
That's the thing that has permits and code, that `writeCoreProposal` is
writing out.

This names what it's creating `CoreEval`. It's not exactly the CoreEval
type but it's 1:1 with a CoreEval so I think it's warranted.

### Security Considerations

n/a

### Scaling Considerations

n/a

### Documentation Considerations

Improves clarity. I don't know what external docs need to be updated.

### Testing Considerations

CI should suffice

### Upgrade Considerations

It should be fully backwards compatible as I maintained the module
exports. I renamed a module but it shouldn't have been deep imported
anywhere.
 - update CosmosChainInfoShape to match type: stakingTokens is optional
 - add IST to vbankAsset
 - inspectLocalBridge
closes #9187

## Description

DRAFT until

- [x] what to do about the way `transfer()` maps chainId to
transferChannel?
 - [x] clean up git history
 - [x] update other facade callers
- [x] integrate more fully with well-known chains: hot-new-chain gets
added and "Nothing breaks."
- [x] what to do with facade contract upgrade test, now that chainInfos
is a caller thing?

### Security Considerations

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

### Upgrade Considerations

<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->
closes: #XXXX
refs: #9299 #9322

## Description

Prepare for #9322 by making any guest use of `E` until then cause an
error. We expect that it might be a while before #9322 is ready for
review. By merging this PR soon, we prevent any guest code or logs that
would commit us to an incompat way of handling `E`.

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

Should document that guests cannot invoke host vows or remotables with
`E` until #9322 , which won't happen immediately.
### Testing Considerations

- [x] need to test what kind of error state this goes into. Should be a
panic, so that an upgrade can unblock guest execution that got stuck
trying to `E`.
### Upgrade Considerations

The point. By making such use of `E` an error now, we ensure that #9322
can proceed without causing any compat problem with committed durable
state.

Co-authored-by: Mathieu Hofman <86499+mhofman@users.noreply.github.com>
@michaelfig
Copy link

michaelfig commented Jun 10, 2024

Superseded by Agoric#9471

mergify bot and others added 2 commits June 10, 2024 17:49
#8624 (comment)

makeBridgeTargetKit can be more general than "IBC transfer", and
is already pulled in separately by vat-transfer.js
@michaelfig michaelfig force-pushed the gibson-9059-vtransfer-tools-simplification branch from af0348e to 9b2bfbb Compare June 10, 2024 17:51
@gibson042
Copy link
Author

Agoric#9471

@gibson042 gibson042 closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants