[api] archive-mode for eth_getBalance & eth_call #4529
Conversation
dustinxie
requested review from
CoderZhi,
Liuhaai,
envestcc and
a team
as code owners
| type intrinsicGasCalculator interface { | ||
| IntrinsicGas() (uint64, error) | ||
| } | ||
|
|
||
| var ( | ||
| // ErrNotFound indicates the record isn't found | ||
| ErrNotFound = errors.New("not found") | ||
| ErrNotFound = errors.New("not found") | ||
| ErrArchiveNotSupported = errors.New("archive-mode not supported") |
There was a problem hiding this comment.
removed L261 comment, the name clears explains what the error is
| } | ||
|
|
||
| func (core *coreServiceReaderWithHeight) Account(addr address.Address) (*iotextypes.AccountMeta, *iotextypes.BlockIdentifier, error) { | ||
| if !core.cs.archiveSupported { |
There was a problem hiding this comment.
bd7ca1f#diff-699e59a265a0599d1a49698bbf378f57a7aeb60957441a257249102ac8785a8dR50 will return error if archive is not supported
|
|
||
| type ( | ||
| // CoreServiceReaderWithHeight is an interface for state reader at certain height | ||
| CoreServiceReaderWithHeight interface { |
There was a problem hiding this comment.
could you explain why we need to create such an interface?
There was a problem hiding this comment.
this interface is created to represent APIs needed for archive-mode query (there will be a total 4-6 of them, 2 enabled in this PR, and more to enable in upcoming PR), so we can keep using existing API without having to add the height parameter to it, that is, we can avoid changes like below
Account(address.Address, height uint64) (*iotextypes.AccountMeta, *iotextypes.BlockIdentifier, error)
api/coreservice.go
Outdated
| @@ -243,13 +246,21 @@ func WithAPIStats(stats *nodestats.APILocalStats) Option { | |||
| } | |||
| } | |||
|
|
|||
| // WithArchiveSupport is the option to enable archive support | |||
| func WithArchiveSupport(enabled bool) Option { | |||
There was a problem hiding this comment.
The enable parameter should be redundant; this option is supposed to enable 'archive mode'.
api/coreservice.go
Outdated
| pendingNonce = state.PendingNonce() | ||
| } | ||
| } else { | ||
| pendingNonce, err = core.ap.GetPendingNonce(addrStr) |
There was a problem hiding this comment.
Recommend to move 'nonce from the actpool` to the Account method, and removing the 'archive bool' parameter would make it clearer.
api/coreservice.go
Outdated
| if err != nil { | ||
| return "", nil, status.Error(codes.Internal, err.Error()) | ||
| } | ||
| // ReadContract() is read-only, if no error returned, we consider it a success | ||
| receipt.Status = uint64(iotextypes.ReceiptStatus_Success) |
There was a problem hiding this comment.
stale code, remove it. refer #4454
| func (core *coreService) simulateExecution( | ||
| ctx context.Context, | ||
| height uint64, | ||
| archive bool, |
There was a problem hiding this comment.
can consider passing 'ws' as a parameter instead of the boolean.
There was a problem hiding this comment.
that's doable, yet would cause code L2004-2008 to be duplicated at these funcs:
func (core *coreService) ReadContract()
func (core *coreService) isGasLimitEnough()
func (core *coreService) SimulateExecution()
func (core *coreService) TraceTransaction()
func (core *coreService) TraceCall()
IMO the current way is more efficient
| if err != nil { | ||
| return nil, nil, status.Error(codes.Internal, err.Error()) | ||
| } | ||
| ws, err = core.sf.WorkingSet(ctx) |
There was a problem hiding this comment.
these lines would be duplicated multiple times if use ws instead of boolean as input
|
Description
as title. Based on #4526
Fixes #(issue)
Type of change
Please delete options that are not relevant.
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
Test Configuration:
Checklist: