refactor: migrate from sdk.Context to Context.Context (1/n) #7058
Conversation
tac0turtle
changed the title
|
ill branch off this pr to continue. |
tac0turtle
requested review from
colin-axner,
AdityaSripal,
damiannolan,
srdtrk,
chatton,
DimitrisJim,
crodriguezvega,
gjermundgaraba and
bznein
as code owners
|
opening this for review so the other prs dont get blocked and build up a backlog |
There was a problem hiding this comment.
So far so good, left a couple of comments to begin with, just nits mostly. Made my way through ica and fee code, will continue reviewing tomorrow.
Thanks @tac0turtle!
I think I can also assist on pushing some follow up commits to the branch for housekeeping trivial stuff
modules/apps/29-fee/keeper/keeper.go
Outdated
| has, err := store.Has(key) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| if !has { | ||
| return "", false | ||
| } | ||
|
|
||
| addr := string(store.Get(key)) | ||
| return addr, true | ||
| addr, err := store.Get(key) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return string(addr), true // TODO: why this cast? |
There was a problem hiding this comment.
ditto on refactor to single Get call and check len bytes.
regarding the TODO - its stored plain string because its an address belonging to a counterparty chain and "in theory" could be non cosmos-sdk addr.
e.g. relayer submits MsgRecvPacket to chainB and looks up a stored address to encode into the acknowledgement. Ack is relayed back to source(chainA) and address is parsed from acknowledgement data and fee is paid out to the account on source chain.
There was a problem hiding this comment.
thank you for the explanation, how come this was the design instead of storingt the raw byets?
There was a problem hiding this comment.
I believe the reasoning was likely around string usage being more human readable, rather than passing around the raw bytes of a foreign address. @AdityaSripal can probably speak to it more tbh
There was a problem hiding this comment.
Made my way through the rest of the diffs. Would like to clarify the api differences in corestore vs storetypes before merging. We now need to handle a bunch of error returns that were previously not there, just wondering how/what will trigger them to be surfaced(see other comment).
Assuming there will be parity available through other service types which we can consume later, in order fully remove sdk.Context and address all the TODOs? Some of the main things that come to mind are:
- ChainID
- BlockTime
- Gas Consumption
- Tx exec mode
Otherwise lgtm! Thank you @tac0turtle. I pushed a couple of consistency and nit follow up commits
…avour of hardcoded string
|
Description
This pr aims to remove sdk.Context in ibcKeeper. This pr will also aim to replace many sdk imports with services from core to allow reducing the dependency graph on the sdk
ref: #5917
Example commit messages:
fix: skip emission of unpopulated memo field in ics20
deps: updating sdk to v0.46.4
chore: removed unused variables
e2e: adding e2e upgrade test for ibc-go/v6
docs: ics27 v6 documentation updates
feat: add semantic version utilities for e2e tests
feat(api)!: this is an api breaking feature
fix(statemachine)!: this is a statemachine breaking fix
-->
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.