Drop go 1.12 and fix some things #1281
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's old and not maintained. Use latest Go for builds and test only with 1.13 and 1.14.
Codecov Report
@@ Coverage Diff @@
## master #1281 +/- ##
==========================================
+ Coverage 61.26% 67.91% +6.64%
==========================================
Files 216 216
Lines 18490 18464 -26
==========================================
+ Hits 11328 12539 +1211
+ Misses 6542 5263 -1279
- Partials 620 662 +42
Continue to review full report at Codecov.
|
AnnaShaleva
requested changes
Aug 7, 2020
pkg/core/blockchain.go
Outdated
| } | ||
| size := io.GetVarSize(t) | ||
| if size > transaction.MaxTransactionSize { | ||
| return errors.Errorf("invalid transaction size = %d. It shoud be less then MaxTransactionSize = %d", io.GetVarSize(t), transaction.MaxTransactionSize) | ||
| return fmt.Errorf("invalid transaction size = %d. It shoud be less then MaxTransactionSize = %d", io.GetVarSize(t), transaction.MaxTransactionSize) |
There was a problem hiding this comment.
We can improve this message by putting size instead of io.GetVarSize(t).
There was a problem hiding this comment.
errors.Errorf -> fmt.Errorf was done automatically, so messages were not improved there, but thanks for comments, I'll improve them where possible.
pkg/smartcontract/param_type.go
Outdated
| @@ -160,7 +161,7 @@ func ParseParamType(typ string) (ParamType, error) { | |||
| case "any": | |||
| return AnyType, nil | |||
| default: | |||
| return UnknownType, errors.Errorf("Unknown contract parameter type: %s", typ) | |||
| return UnknownType, fmt.Errorf("Unknown contract parameter type: %s", typ) | |||
There was a problem hiding this comment.
Previous errors start with lowercase letter, although it's not so important.
pkg/smartcontract/parameter.go
Outdated
| @@ -94,7 +94,7 @@ func (p Parameter) MarshalJSON() ([]byte, error) { | |||
| case InteropInterfaceType, AnyType: | |||
| resultRawValue = nil | |||
| default: | |||
| resultErr = errors.Errorf("Marshaller for type %s not implemented", p.Type) | |||
| resultErr = fmt.Errorf("Marshaller for type %s not implemented", p.Type) | |||
pkg/smartcontract/parameter.go
Outdated
| @@ -183,7 +183,7 @@ func (p *Parameter) UnmarshalJSON(data []byte) (err error) { | |||
| // stub, ignore value, it can only be null | |||
| p.Value = nil | |||
| default: | |||
| return errors.Errorf("Unmarshaller for type %s not implemented", p.Type) | |||
| return fmt.Errorf("Unmarshaller for type %s not implemented", p.Type) | |||
pkg/smartcontract/parameter.go
Outdated
| @@ -412,7 +412,7 @@ func NewParameterFromString(in string) (*Parameter, error) { | |||
| } | |||
| // We currently do not support following types: | |||
| if res.Type == ArrayType || res.Type == MapType || res.Type == InteropInterfaceType || res.Type == VoidType { | |||
| return nil, errors.Errorf("Unsupported contract parameter type: %s", res.Type) | |||
| return nil, fmt.Errorf("Unsupported contract parameter type: %s", res.Type) | |||
Comment on lines
+529
to
+533
| maxBlockSystemFee := p.GetMaxBlockSystemFeeInternal(d) | ||
| if maxBlockSystemFee < tx.SystemFee { | ||
| return fmt.Errorf("transaction's fee can't exceed maximum block system fee %d", maxBlockSystemFee) | ||
| } |
There was a problem hiding this comment.
The comment to CheckPolicy should also be updated according to these changes.
roman-khimov
force-pushed
the
drop-go-1.12-and-fix-some-things
branch
from
433e9aa
to
ff66307
Compare
It's not needed any more with Go 1.13 as we have wrapping/unwrapping in base packages. All errors.Wrap calls are replaced with fmt.Errorf, some strings are improved along the way.
It doesn't really change anything in most of the cases, but it's a useful habit anyway. Fix #350.
ErrAlreadyExists should be returned for ErrDup, not ErrConflict.
Just in case.
It no longer depends on blockchain state and there can't ever be an error, in fact we can always iterate over signers, so copying these hashes doesn't make much sense at all as well as sorting arrays in verifyTxWitnesses (witnesses order must match signers order).
Simplify things a bit.
We were checking blocked accounts twice which is obviously excessive. We also have our accounts sorted, so we can rely on that in CheckPolicy(). It also doesn't make much sense to check MaxBlockSystemFee in Blockchain code, policy contract can handle that.
roman-khimov
force-pushed
the
drop-go-1.12-and-fix-some-things
branch
from
ff66307
to
d5a9d80
Compare
fyrchik
approved these changes
Aug 7, 2020
AnnaShaleva
approved these changes
Aug 7, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We no longer need to support Go 1.12, so we can fix #349 and #350. At the same time I've noticed some other minor problems, so they were also fixed (albeit it makes this PR somewhat unfocused).