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

Initial implementation of IBC Precompile #37

Closed
wants to merge 4 commits into from

Conversation

evgeniy-scherbina
Copy link

@evgeniy-scherbina evgeniy-scherbina commented Feb 22, 2024

Based on #32

@evgeniy-scherbina evgeniy-scherbina marked this pull request as ready for review February 26, 2024 15:17
func (s *StateDB) Context() context.Context {
// s.ctx.CurrentCtx() replaced s.ctx after rebasing
// TODO(yevhenii): is it correct?
return s.ctx.CurrentCtx()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup this is the correct ctx of the active snapshot to use for making state changes

@@ -464,3 +472,12 @@ func (s *StateDB) SetError(err error) {

s.sdkError = err
}

func (s *StateDB) IBCTransfer(goCtx context.Context, msg *ibctransfertypes.MsgTransfer) (*ibctransfertypes.MsgTransferResponse, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit out of place for IBCTransfer() to be in the statedb, but does seem to make it easier to add access to the keepers without breaking more things in geth.

To me I think it makes a bit more sense to pass in additional keepers to geth vm.NewEVM() then provide the matching keeper interface in contract.accessibleState. Since it doesn't seem needed by StateDB or the Ethermint keepers? (pls correct me if wrong)

Just cuts out an extra step passing the keepers around like so:
Kava -> Ethermint -> EVM -> Custom precompile
Kava -> EVM -> Custom precompile

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds reasonable, the only downside is changing func NewEVM(...) *EVM signature will be a bit painful, it's widely used across go-ethereum codebase (40 direct occurrences)
cc @nddeluca

@evgeniy-scherbina evgeniy-scherbina force-pushed the yevhenii/ibc-precompile branch 3 times, most recently from d5fc637 to 457e857 Compare March 15, 2024 18:25
Copy link
Author

@Mergifyio refresh

Copy link

mergify bot commented Apr 19, 2024

refresh

✅ Pull request refreshed

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.

2 participants