[action] remove AbstractAction from all actions #4380
Conversation
dustinxie
requested review from
CoderZhi,
Liuhaai,
envestcc and
a team
as code owners
| IntrinsicGas() (uint64, error) | ||
| SetEnvelopeContext(*AbstractAction) |
There was a problem hiding this comment.
now it will use AbstractAction in the envelope, not in each action, so this is no longer needed
There was a problem hiding this comment.
Cost may serve as an optional implementation method. If the payload is not implemented, it will be price * intrinsicGas; if implemented, the result implemented by the payload will be used.
For example, execution should be price * gasLimit, and CreateStake should be price * intrinsicGas + stake amount.
There was a problem hiding this comment.
Cost is implemented by every action, and used by actpool
now the actions (like Execution, CreateStake) does not have the gasPrice field, so have to move the implementation up to the envelope struct
There was a problem hiding this comment.
I see, but I prefer define an interface for payload to calc cost in envelop side, instead of calc by type explicitly
action/envelope.go
Outdated
| @@ -18,7 +18,7 @@ import ( | |||
| type ( | |||
| // Envelope defines an envelope wrapped on action with some envelope metadata. | |||
| Envelope interface { | |||
| Version() uint32 | |||
| CommonAction() AbstractAction | |||
There was a problem hiding this comment.
so far, keep this as a struct
in the next PR, will convert to an interface (and have the 4 types of transactions implement it)
There was a problem hiding this comment.
can we just inherit the AbstractAction? I didn't see the necessity of making it as a method
There was a problem hiding this comment.
yes, removed in latest code
| if amount, ok := elp.payload.(hasAmount); ok { | ||
| cost.Add(cost, amount.Amount()) | ||
| } | ||
| return cost, nil |
There was a problem hiding this comment.
this is the same implementation as Cost() in each action now
| @@ -448,7 +448,7 @@ func (p *Protocol) handle(ctx context.Context, act action.Action, csm CandidateS | |||
| case *action.CandidateTransferOwnership: | |||
| rLog, tLogs, err = p.handleCandidateTransferOwnership(ctx, act, csm) | |||
| case *action.MigrateStake: | |||
| logs, tLogs, gasConsumed, gasToBeDeducted, err = p.handleStakeMigrate(ctx, act, csm) | |||
| logs, tLogs, gasConsumed, gasToBeDeducted, err = p.handleStakeMigrate(ctx, elp, csm) | |||
There was a problem hiding this comment.
pass act, nonce, limit as parameters
| actLogs := make([]*action.Log, 0) | ||
| transferLogs := make([]*action.TransactionLog, 0) | ||
| insGas, err := act.IntrinsicGas() | ||
| func (p *Protocol) handleStakeMigrate(ctx context.Context, elp action.Envelope, csm CandidateStateManager) ([]*action.Log, []*action.TransactionLog, uint64, uint64, error) { |
There was a problem hiding this comment.
handleStakeMigrate(ctx, common, act, csm)
As the envelope has been opened, passing the common and act as separate parameters is more reasonable
There was a problem hiding this comment.
now Envelope does not have a method to get the common AbstractAction part, we can do this once next PR converts AbstractAction to an interface and becomes a member var of Envelope
filed a TODO
| case *CandidateRegister: | ||
| extraAmount = act.Amount() | ||
| if _, ok := elp.payload.(gasLimitForCost); ok { | ||
| gas = elp.GasLimit() |
There was a problem hiding this comment.
in what case, gas limit is not calculated as cost?
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4380 +/- ##
==========================================
- Coverage 75.86% 75.26% -0.60%
==========================================
Files 361 368 +7
Lines 30116 30018 -98
==========================================
- Hits 22846 22594 -252
- Misses 6134 6262 +128
- Partials 1136 1162 +26 ☔ View full report in Codecov by Sentry. |
Description
following PR (#4297), this is to remove AbstractAction from all actions.
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: