feat(vibc): preparation for Agoric ibc-hooks #9030
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JimLarson
approved these changes
Mar 5, 2024
| // Filled out by `WithScope` | ||
| scopedKeeper types.ScopedKeeper | ||
| storeKey storetypes.StoreKey | ||
| pushAction vm.ActionPusher | ||
| } | ||
|
|
||
| // NewKeeper creates a new dIBC Keeper instance |
There was a problem hiding this comment.
Suggested change
| // NewKeeper creates a new dIBC Keeper instance | |
| // NewKeeper creates a new vIBC Keeper instance |
| "github.com/Agoric/agoric-sdk/golang/cosmos/x/vibc/types" | ||
| ) | ||
|
|
||
| var ( | ||
| _ porttypes.ICS4Wrapper = Keeper{} |
There was a problem hiding this comment.
This check is redundant with the return type of GetICS4Wrapper(), but might still be useful for documentation.
| } | ||
| } | ||
|
|
||
| func (k Keeper) GetBalance(ctx sdk.Context, addr sdk.AccAddress, denom string) sdk.Coin { | ||
| return k.bankKeeper.GetBalance(ctx, addr, denom) | ||
| func (k Keeper) WithScope(storeKey storetypes.StoreKey, scopedKeeper types.ScopedKeeper, pushAction vm.ActionPusher) Keeper { |
There was a problem hiding this comment.
Godoc comments on all public methods, please, even if they're trivial.
| @@ -67,7 +81,7 @@ func (k Keeper) GetChannel(ctx sdk.Context, portID, channelID string) (channelty | |||
|
|
|||
| // ChanOpenInit defines a wrapper function for the channel Keeper's function | |||
There was a problem hiding this comment.
Suggested change
| // ChanOpenInit defines a wrapper function for the channel Keeper's function | |
| // ReceiveChanOpenInit defines a wrapper function for the channel Keeper's function |
| return k.WriteAcknowledgement(ctx, chanCap, packet, ack) | ||
| } | ||
|
|
||
| func (k Keeper) WriteAcknowledgement(ctx sdk.Context, chanCap *capability.Capability, packet ibcexported.PacketI, ack ibcexported.Acknowledgement) error { |
There was a problem hiding this comment.
godoc for all exported methods, even if trivial.
| @@ -157,18 +186,19 @@ func (k Keeper) ChanCloseInit(ctx sdk.Context, portID, channelID string) error { | |||
|
|
|||
| // BindPort defines a wrapper function for the port Keeper's function in | |||
There was a problem hiding this comment.
Suggested change
| // BindPort defines a wrapper function for the port Keeper's function in | |
| // ReceiveBindPort defines a wrapper function for the port Keeper's function in |
| if ok { | ||
| return fmt.Errorf("port %s is already bound", portID) | ||
| } | ||
| cap := k.portKeeper.BindPort(ctx, portID) | ||
| return k.ClaimCapability(ctx, cap, host.PortPath(portID)) | ||
| return k.ClaimCapability(ctx, cap, portPath) | ||
| } | ||
|
|
||
| // TimeoutExecuted defines a wrapper function for the channel Keeper's function |
There was a problem hiding this comment.
Suggested change
| // TimeoutExecuted defines a wrapper function for the channel Keeper's function | |
| // ReceiveTimeoutExecuted defines a wrapper function for the channel Keeper's function |
| } | ||
|
|
||
| func (ir Receiver) Receive(cctx context.Context, str string) (ret string, err error) { | ||
| // fmt.Println("ibc.go downcall", str) |
There was a problem hiding this comment.
Consider removing or replacing with debug-level logging.
| msg := new(portMessage) | ||
| err = json.Unmarshal([]byte(str), &msg) | ||
| if err != nil { | ||
| return ret, err |
There was a problem hiding this comment.
I know ret is really initialized to "", but it feels uninitialized. :-)
Suggested change
| return ret, err | |
| return "", err |
| stringToOrder(msg.Order), msg.Hops, msg.Version, | ||
| ) | ||
| if err == nil { | ||
| ret = "true" |
There was a problem hiding this comment.
Consider using a constant for "true" so you won't get tripped up on a misspelling.
Also consider setting it after the switch instead:
if ret == "" && err == nil {
ret = "true" // or a defined constant
}
michaelfig
added
the
automerge:no-update
michaelfig
force-pushed
the
mfig-ibc-revamp
branch
from
cb94897 to
c2b36dc
Compare
gibson042
added a commit
that referenced
this pull request
Apr 19, 2024
## Description Includes commits from the following PRs: * #9030 (partial cherry-pick; refactors go.mod) * #9251 * #9263 Constructed using the following `git rebase -i HEAD` todo list: ``` # pull request #9030 branch mfig-ibc-revamp ## Replace this commit with just its refactoring of golang/cosmos/go.mod ## and subsequent `go mod tidy`. edit fff392b build(cosmos): add `ibc-go` fork and `go mod tidy` exec git commit --amend --reset-author -m 'build(cosmos): Refactor go.mod to match master' # pull request #9251 branch 9224-u15-changes label base-9224-u15-changes ## Resolve conflict using `go mod tidy`. pick 548e758 fix: various fixes and features for u15 label 9224-u15-changes reset base-9224-u15-changes merge -C c1ec5f1 9224-u15-changes # fix: various fixes and features for u15 (#9251) # pull request #9263 branch 9224-abci-flag-tests label base-9224-abci-flag-tests pick 0d9a63d test: pick up cosmos-sdk with more abci flag tests label 9224-abci-flag-tests reset base-9224-abci-flag-tests merge -C 947973c 9224-abci-flag-tests # test: pick up cosmos-sdk with more abci flag tests (#9263) ``` ### Security Considerations None in particular. ### Scaling Considerations This brings in the ability to switch between "local" and "committing" query client behavior. ### Documentation Considerations Adopted by reference. ### Testing Considerations No additions. ### Upgrade Considerations To be included in agoric-upgrade-15.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #9029
Description
As it introduces new bridge message types and new fields, this is not a pure refactor. However, the changes should be straightforward to a Golang reviewer with basic knowledge of Cosmos SDK modules.
Security Considerations
Does not inherently introduce new calls, but provides infrastructure to do so in a future PR, such as #8624.
Scaling Considerations
n/a
Documentation Considerations
Will be documented once
vtransferhas proven viable.Testing Considerations
Will test as part of
vtransfer.Upgrade Considerations
No additional considerations. Vats and contracts leveraging Agoric ibc-hooks will need to consider their own upgradability.