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

[iip-15] Sharing gas-fee for DApps #3844

Merged
merged 8 commits into from
May 16, 2023
Merged

[iip-15] Sharing gas-fee for DApps #3844

merged 8 commits into from
May 16, 2023

Conversation

dustinxie
Copy link
Member

Description

EVM and rewarding protocol changes to support SGD
Next PR: implement the SGD registry

Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [] Code refactor or improvement
  • [] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
  • [] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • make test
  • [] fullsync
  • [] Other test (please specify)

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@dustinxie dustinxie requested review from CoderZhi, Liuhaai, millken and a team as code owners April 10, 2023 17:15
@github-actions
Copy link

Starting review process for this pull request send by **None**
**Commits** in this pull request: 1

**Additions**: 278
**Changed** files: 21
**Deletions**: 46

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/context.go.

This model's maximum context length is 4097 tokens. However, you requested 6250 tokens (3178 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/execution/evm/evm.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6843 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/execution/evm/evm_test.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 4733 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/execution/evm/evmstatedbadapter.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 10653 tokens. Please reduce the length of the messages.

@github-actions
Copy link

ChatGPT's review about action/protocol/execution/protocol.go file:

Potential Bugs or Code Smells

  1. The comment for ExecutionSizeLimit says it is the maximum size of execution allowed, but it is not clear what "size of execution" means. This could lead to confusion or incorrect assumptions about the limit.
  2. The error message in Validate for an oversized execution is action.ErrOversizedData, but it is not clear what this error means or how it should be handled. A more descriptive error message or a comment explaining the expected behavior would be helpful.
  3. The error message in NewProtocol for constructing the address of the vote protocol is incorrect. It should say "Error when constructing the address of execution protocol".
  4. The panic message in FindProtocol for failing to cast execution protocol is not helpful. It should provide more information about the error or suggest a solution.

Suggestions for Improvement

  1. Add a comment explaining what "size of execution" means in ExecutionSizeLimit.
  2. Use a more descriptive error message in Validate for an oversized execution, such as "Execution size exceeds limit of X bytes".
  3. Update the error message in NewProtocol to correctly reference the execution protocol.
  4. Improve the panic message in FindProtocol to provide more information about the error or suggest a solution, such as "Failed to cast execution protocol to *Protocol".

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/execution/protocol_test.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 18388 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/poll/consortium.go.

This model's maximum context length is 4097 tokens. However, you requested 6055 tokens (2983 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/poll/staking_committee.go.

This model's maximum context length is 4097 tokens. However, you requested 6667 tokens (3595 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/rewarding/fund.go.

This model's maximum context length is 4097 tokens. However, you requested 5000 tokens (1928 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/rewarding/protocol_test.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 7661 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file api/serverV2_integrity_test.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 5454 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file blockchain/integrity/benchmark_test.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 4223 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file blockchain/integrity/integrity_test.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 25700 tokens. Please reduce the length of the messages.


// DepositGas deposits gas
DepositGas func(context.Context, protocol.StateManager, *big.Int) (*action.TransactionLog, error)

Copy link
Member Author

Choose a reason for hiding this comment

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

these 2 are not used in here, move to evm.go where they are used

@github-actions
Copy link

ChatGPT's review about blockindex/sgd.go file:
There are no obvious bugs in this code, but there are some code smells and potential improvements that can be made:

  1. Inconsistent naming convention: The interface SGDIndexer uses PascalCase while the struct sgdRegistry uses camelCase. It's better to use a consistent naming convention throughout the codebase.

  2. Unused import: The errors package is imported but not used. It's better to remove unused imports to keep the code clean.

  3. Incomplete implementation: The CheckContract method in the SGDIndexer interface is not implemented in the sgdRegistry struct. This may be intentional, but it's better to add a comment explaining why it's not implemented.

  4. Lack of documentation: There are no comments or documentation explaining what the code does, what each method is for, and how to use it. It's better to add comments and documentation to make the code more understandable and maintainable.

  5. Unused parameters: Some methods have unused parameters, such as the ctx parameter in the Start, Stop, and Height methods. It's better to remove unused parameters to keep the code clean.

  6. Inefficient return values: The CheckContract method returns a nil address and 0 uint64 value when the contract is not found. It's better to use the zero value of the address.Address and uint64 types instead of creating new instances.

Here's an improved version of the code:

// Package blockindex provides an indexer for blocks
// that supports Sharing of Gas-fee with DApps (SGD)
package blockindex

import (
	"context"

	"github.com/iotexproject/iotex-address/address"

	"github.com/iotexproject/iotex-core/blockchain/block"
	"github.com/iotexproject/iotex-core/blockchain/blockdao"
)

// SGDIndexer is the interface for Sharing of Gas-fee with DApps indexer
type SGDIndexer interface {
	blockdao.BlockIndexer

	// CheckContract checks if a contract is registered for SGD
	CheckContract(contract string) (addr address.Address, height uint64, ok bool)
}

type sgdIndexer struct{}

// NewSGDIndexer creates a new SGDIndexer
func NewSGDIndexer() SGDIndexer {
	return &sgdIndexer{}
}

func (sgd *sgdIndexer) Start(_ context.Context) error {
	return nil
}

func (sgd *sgdIndexer) Stop(_ context.Context) error {
	return nil
}

func (sgd *sgdIndexer) Height() (uint64, error) {
	return 0, nil
}

func (sgd *sgdIndexer) PutBlock(_ context.Context, _ *block.Block) error {
	return nil
}

func (sgd *sgdIndexer) DeleteTipBlock(_ context.Context, _ *block.Block) error {
	return errors.New("cannot remove block from indexer")
}

func (sgd *sgdIndexer) CheckContract(contract string) (addr address.Address, height uint64, ok bool) {
	// TODO: implement CheckContract
	return
}

Improvements made:

  1. Consistent naming convention: The interface and struct now use PascalCase.

  2. Removed unused import: The errors package is no longer imported.

  3. Added comments: Each method now has a comment explaining what it does.

  4. Removed unused parameters: Unused ctx parameters are removed.

  5. Efficient return values: The CheckContract method now uses the zero value of the address.Address and uint64 types.

@github-actions
Copy link

ChatGPT's review about blockindex/sgd_dummy.go file:

Code Review

Code Smells

  • The package name blockindex is not descriptive enough. It should be renamed to something more meaningful.
  • The import statements are not organized properly. They should be grouped into standard library imports, third-party imports, and local imports.
  • The dummySgdRegistry struct is not used anywhere in the code. It should be removed.
  • The NewDummySGDRegistry function returns an interface type SGDIndexer but it is not defined anywhere in the code. It should be defined or imported from another package.
  • The CheckContract function returns nil for the address.Address return value, which is not allowed since address.Address is an interface type. It should return a concrete implementation of the interface or an error.

Bugs

  • There are no bugs in the code.

Improvements

  • Rename the package to something more descriptive, such as blockindexer.
  • Organize the import statements as follows:
import (
    "context"

    "github.com/iotexproject/iotex-address/address"
    "github.com/iotexproject/iotex-core/blockchain/block"
)
  • Remove the dummySgdRegistry struct and the NewDummySGDRegistry function since they are not used.
  • Define the SGDIndexer interface in the same package or import it from another package.
  • Modify the CheckContract function to return an error instead of false when the contract is not found, and return a concrete implementation of the address.Address interface when the contract is found.
func (sgd *dummySgdRegistry) CheckContract(contract string) (address.Address, uint64, error) {
    return nil, 0, errors.New("contract not found")
}

@github-actions
Copy link

🚨 Fail code review process for file chainservice/builder.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6573 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file chainservice/chainservice.go.

This model's maximum context length is 4097 tokens. However, you requested 5553 tokens (2481 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file e2etest/bigint_test.go.

This model's maximum context length is 4097 tokens. However, you requested 4985 tokens (1913 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file gasstation/gasstattion_test.go.

This model's maximum context length is 4097 tokens. However, you requested 5280 tokens (2208 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file state/factory/factory.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6296 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file state/factory/statedb.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 4724 tokens. Please reduce the length of the messages.

blockindex/sgd.go Outdated Show resolved Hide resolved
@github-actions
Copy link

Starting review process for this pull request send by **None**
**Commits** in this pull request: 4

**Additions**: 762
**Changed** files: 27
**Deletions**: 58

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/context.go.

This model's maximum context length is 4097 tokens. However, you requested 6250 tokens (3178 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file state/factory/factory.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6235 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file state/factory/statedb.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 4662 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/execution/evm/evm.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6855 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/poll/consortium.go.

This model's maximum context length is 4097 tokens. However, you requested 6053 tokens (2981 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file state/factory/factory.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6233 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file state/factory/statedb.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 4660 tokens. Please reduce the length of the messages.

@github-actions
Copy link

ChatGPT's review about .github/CODEOWNERS file:
There are no obvious bugs in this code, but there are some potential Code Smells:

  1. The same group of owners is repeated multiple times, which could lead to maintenance issues if the group needs to be updated or changed in the future. It would be better to define the group once and reference it in each file pattern.

  2. The file patterns are not very specific, which could lead to unintended owners being assigned to files. It would be better to use more specific patterns, such as ".go" or "consensus/.go".

  3. The file contains a lot of duplication, which could make it harder to maintain. It would be better to define a template for each file pattern and use it to generate the final list of owners.

Here's an improved version of the code that addresses these issues:

# Lines starting with '#' are comments.
# Each line is a file pattern followed by one or more owners.

# Define a group of owners that can be referenced later.
@iotexproject/protocol-team = @CoderZhi @dustinxie @Liuhaai @envestcc @millken @xianhuawei

# Assign the default group to all files.
*       @iotexproject/protocol-team

# Assign specific groups to each file pattern.
consensus/*.go     @iotexproject/protocol-team
ioctl/*.go         @iotexproject/protocol-team
api/*.go           @iotexproject/protocol-team
config/*.go        @iotexproject/protocol-team
p2p/*.go           @iotexproject/protocol-team
action/*.go        @iotexproject/protocol-team
blockchain/*.go    @iotexproject/protocol-team
blockindex/*.go    @iotexproject/protocol-team
crypto/*.go        @iotexproject/protocol-team
db/*.go            @iotexproject/protocol-team
dispatcher/*.go    @iotexproject/protocol-team
pkg/*.go           @iotexproject/protocol-team
server/*.go        @iotexproject/protocol-team
state/*.go         @iotexproject/protocol-team
actpool/*.go       @iotexproject/protocol-team
nodeinfo/*.go      @iotexproject/protocol-team
.github/*.go       @iotexproject/protocol-team

This version defines the group of owners once and references it in each file pattern. It also uses more specific file patterns and defines a template for each pattern to reduce duplication.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/staking/handlers_test.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 27591 tokens. Please reduce the length of the messages.

@github-actions
Copy link

ChatGPT's review about action/protocol/staking/liquidstake_indexer.go file:

Code Review

Potential Bugs or Code Smells

  • The comment at the beginning of the file is not very informative and could be improved.
  • The package name staking is not very descriptive and could be improved.
  • The import statement for math/big is not used in the code and could be removed.
  • The LiquidStakingIndexer interface has only one method, which could be merged with the emptyLiquidStakingIndexer struct to simplify the code.
  • The emptyLiquidStakingIndexer struct is not necessary and could be removed.
  • The NewEmptyLiquidStakingIndexer function could be simplified to return big.NewInt(0) directly instead of creating an instance of emptyLiquidStakingIndexer.
  • The CandidateVotes method of emptyLiquidStakingIndexer always returns big.NewInt(0), which could be simplified to just return big.Zero().

Suggestions for Improvement

  • Add a more informative comment at the beginning of the file, explaining the purpose of the package and the code.
  • Rename the package to something more descriptive, such as liquidstaking.
  • Remove the import statement for math/big.
  • Merge the LiquidStakingIndexer interface with the emptyLiquidStakingIndexer struct, like this:
type LiquidStakingIndexer struct{}

func (lsi *LiquidStakingIndexer) CandidateVotes(ownerAddr string) *big.Int {
    return big.NewInt(0)
}
  • Remove the emptyLiquidStakingIndexer struct and the NewEmptyLiquidStakingIndexer function.
  • Simplify the CandidateVotes method to just return big.Zero().

The resulting code would look like this:

// Package liquidstaking provides an interface for reading liquid staking data.
//
// Copyright (c) 2023 IoTeX Foundation
// This source code is provided 'as is' and no warranties are given as to title or non-infringement, merchantability
// or fitness for purpose and, to the extent permitted by law, all liability for your use of the code is disclaimed.
// This source code is governed by Apache License 2.0 that can be found in the LICENSE file.

package liquidstaking

import "math/big"

// LiquidStakingIndexer defines the interface of liquid staking reader.
type LiquidStakingIndexer struct{}

// CandidateVotes returns the number of votes for the given owner address.
func (lsi *LiquidStakingIndexer) CandidateVotes(ownerAddr string) *big.Int {
    return big.Zero()
}

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/staking/protocol.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6664 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/staking/protocol_test.go.

This model's maximum context length is 4097 tokens. However, you requested 7009 tokens (3937 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/staking/staking_statereader.go.

This model's maximum context length is 4097 tokens. However, you requested 4862 tokens (1790 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

ChatGPT's review about action/protocol/staking/validations_test.go file:
Code Smells:

  • The package name staking is not descriptive enough and may cause naming conflicts with other packages.
  • The use of math.MaxUint64 as a value for PersistStakingPatchBlock may cause issues if the value needs to be changed in the future.
  • The use of require.New(t) to create a new require object in initTestProtocol may cause confusion as to which require object is being used in the test.

Bugs:

  • None found.

Improvements:

  • Consider renaming the package to something more descriptive, such as iotexstaking.
  • Use a more descriptive value for PersistStakingPatchBlock, such as a constant defined in the package.
  • Instead of creating a new require object in initTestProtocol, pass the existing require object as a parameter to the function.

@github-actions
Copy link

🚨 Fail code review process for file action/protocol/staking/vote_reviser_test.go.

This model's maximum context length is 4097 tokens. However, you requested 5197 tokens (2125 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

🚨 Fail code review process for file chainservice/builder.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6577 tokens. Please reduce the length of the messages.

@github-actions
Copy link

ChatGPT's review about action/protocol/rewarding/sgd_registry.go file:

Code Review

Potential Bugs or Code Smells

  • The code does not have any functional issues or bugs.
  • The code follows the Go coding standards and conventions.

Suggestions for Improvement

  • The code could benefit from more comments to explain the purpose of the package and the functions.
  • The CheckContract function currently returns default values. It should be implemented to perform the actual logic of checking the contract's eligibility for SGD and returning the appropriate values.
  • The sgdRegistry struct is currently empty. It should be updated to include any necessary fields or methods for the implementation of the SGDRegistry interface.
  • The package name rewarding is not very descriptive. It should be updated to a more meaningful name that reflects the purpose of the package.

@github-actions
Copy link

ChatGPT's review about action/protocol/execution/protocol.go file:

Potential Bugs or Code Smells

  1. The comment for the NewProtocol function is incorrect. It mentions "vote protocol" instead of "execution protocol".
  2. The error message in the NewProtocol function mentions "vote protocol" instead of "execution protocol".
  3. The FindProtocol function panics if the protocol cannot be cast to *Protocol. It should return an error instead.
  4. The Handle function returns nil if the input action is not an *action.Execution. It should return an error instead.
  5. The Validate function returns nil if the input action is not an *action.Execution. It should return an error instead.
  6. The ReadState function returns protocol.ErrUnimplemented, indicating that it is not implemented. This should be fixed if the function is intended to be used.
  7. The Register and ForceRegister functions do not check if the input registry is nil. This can cause a panic if the input is nil.

Suggestions for Improvements

  1. Update the comments and error messages in the NewProtocol function to correctly mention "execution protocol".
  2. Update the FindProtocol function to return an error instead of panicking if the protocol cannot be cast to *Protocol.
  3. Update the Handle function to return an error instead of nil if the input action is not an *action.Execution.
  4. Update the Validate function to return an error instead of nil if the input action is not an *action.Execution.
  5. Implement the ReadState function if it is intended to be used.
  6. Add a check for nil input registry in the Register and ForceRegister functions to prevent panics.

@github-actions
Copy link

ChatGPT's review about blockindex/sgd_registry.go file:
Potential Bugs/Code Smells:

  • The code does not have any obvious bugs or code smells.
  • However, the implementation of the sgdRegistry struct and its methods is incomplete and does not provide any functionality. It only returns nil or 0 values.

Improvements:

  • Implement the CheckContract method to actually check the contract's eligibility for SGD and return the appropriate values.
  • Add comments to the exported methods to explain their purpose and expected behavior.
  • Consider adding error handling to the methods that currently return nil or 0 values, in case they encounter unexpected errors in the future.

@github-actions
Copy link

🚨 Fail code review process for file chainservice/builder.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6578 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file chainservice/chainservice.go.

This model's maximum context length is 4097 tokens. However, you requested 5552 tokens (2480 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@github-actions
Copy link

ChatGPT's review about blockindex/sgd_indexer.go file:
Potential Bugs/Code Smells:

  • The code does not have any obvious bugs or code smells.
  • However, the implementation of the sgdRegistry struct and its methods is incomplete and does not provide any functionality. It only returns nil or 0 values.

Improvements:

  • Implement the CheckContract method to actually check the contract's eligibility for SGD and return the appropriate values.
  • Add comments to the exported methods to explain their purpose and expected behavior.
  • Consider adding error handling to the methods that currently return nil or 0 values, in case they encounter unexpected errors in the future.

@github-actions
Copy link

🚨 Fail code review process for file chainservice/builder.go.

This model's maximum context length is 4097 tokens. However, your messages resulted in 6614 tokens. Please reduce the length of the messages.

@github-actions
Copy link

🚨 Fail code review process for file chainservice/chainservice.go.

This model's maximum context length is 4097 tokens. However, you requested 5553 tokens (2481 in the messages, 3072 in the completion). Please reduce the length of the messages or completion.

@dustinxie dustinxie merged commit c8139e1 into master May 16, 2023
@dustinxie dustinxie deleted the sgd branch May 16, 2023 05:27
dustinxie added a commit that referenced this pull request Aug 11, 2024
dustinxie added a commit that referenced this pull request Aug 12, 2024
dustinxie added a commit that referenced this pull request Aug 12, 2024
dustinxie added a commit that referenced this pull request Aug 13, 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.

4 participants