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 Stateful Precompile Contracts #4

Closed
wants to merge 1 commit into from

Conversation

evgeniy-scherbina
Copy link

@evgeniy-scherbina evgeniy-scherbina commented Jan 5, 2024

Moved to #5

@evgeniy-scherbina evgeniy-scherbina force-pushed the yevhenii/precompile-v2 branch 2 times, most recently from 99d95a4 to 3b338ea Compare January 9, 2024 19:49
@evgeniy-scherbina evgeniy-scherbina changed the title Yevhenii/precompile v2 Initial implementation of Stateful Precompile Contracts Jan 9, 2024
@evgeniy-scherbina evgeniy-scherbina marked this pull request as ready for review January 9, 2024 21:42
@@ -231,6 +237,26 @@ func (c *sha256hash) Run(input []byte) ([]byte, error) {
return h[:], nil
}

type sum3 struct{}

Copy link

Choose a reason for hiding this comment

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

Is it worth splitting the specific pre-compiles into a separate file per precompile (thinking yes if there are going to be 10's of pre-compiles, no if less)

Copy link
Author

Choose a reason for hiding this comment

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

This is native stateless precompile, they all are placed in one file here: https://github.com/Kava-Labs/go-ethereum/blob/master/core/vm/contracts.go
I just extended it with one more example stateless smart contract.

New stateful smart contracts is placed one per golang package.

fyi: PR was moved to #10

@@ -224,7 +243,8 @@ func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas
}

if isPrecompile {
ret, gas, err = RunPrecompiledContract(p, input, gas)
// TODO(yevhenii): deduct gas?
Copy link

Choose a reason for hiding this comment

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

Worth adding test cases for this, I think the cosmos-sdk side of things will handle tracking and deducting actual gas (with a possible assist from statedb @drklee3 ?) but having unit and integration tests around what happens gas wise when a block with a failed precompile tx is executed are a useful foundation for testing the feature now and deeper as needed going forward

Copy link
Author

Choose a reason for hiding this comment

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

fyi: PR was moved to #10

gas deduction changed a bit in a new PR, also I added some tests related to gas calculations

}

// NewStatefulPrecompileContract generates new StatefulPrecompile using [functions] as the available functions and [fallback]
// as an optional fallback if there is no input data. Note: the selector of [fallback] will be ignored, so it is required to be left empty.
Copy link

Choose a reason for hiding this comment

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

Guessing there's some higher level function we have to satisfy otherwise we could just drop this unused fallback variable?

Copy link
Author

Choose a reason for hiding this comment

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

I already dropped fallback in a new version of PR, though I think we still need to discuss either we need fallback function or no


func CalculateFunctionSelector(functionSignature string) []byte {
hash := crypto.Keccak256([]byte(functionSignature))
return hash[:4]
Copy link

Choose a reason for hiding this comment

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

Why just the last 4 bytes / worth a code comment explaining it for future readers?

Copy link
Author

Choose a reason for hiding this comment

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

@galxy25 This function was improved in a new PR

Copy link

@galxy25 galxy25 left a comment

Choose a reason for hiding this comment

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

LGTM - few minor / non-blocking comments

@evgeniy-scherbina
Copy link
Author

Moved to #5

@evgeniy-scherbina evgeniy-scherbina added duplicate This issue or pull request already exists deprecated labels Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants