Revert BaseAccount back to cosmos-sdk #588
Conversation
|
|
torao
force-pushed
the
feature/revert-pubkey-fields-back-to-cosmos-style
branch
from
ecd4482 to
8bbec1e
Compare
Codecov Report
@@ Coverage Diff @@
## main #588 +/- ##
==========================================
- Coverage 57.55% 57.53% -0.02%
==========================================
Files 796 796
Lines 87814 87526 -288
==========================================
- Hits 50543 50361 -182
+ Misses 34100 34026 -74
+ Partials 3171 3139 -32
|
There was a problem hiding this comment.
Let's revert too:
- https://github.com/line/lbm-sdk/pull/320/files#diff-085b7c8caa4b62d6ef4bb7a64252bdd927015fe83cbd4f3c62ec58d8741f0d73R106
- https://github.com/line/lbm-sdk/pull/320/files#diff-876463e36c6e6e6ca0eed52e420d4225e3fca76cf84807faba3a8ebf9ba77fcfR68
- etc.
I've checked related PRs:
- https://github.com/line/lbm-sdk/pull/320/files
- https://github.com/line/lbm-sdk/pull/326/files
- https://github.com/line/lbm-sdk/pull/517/files
I think we can more revert. I'll recommend you the reverts after I pick them.
| @@ -124,7 +124,7 @@ func TestBaseApp_BlockGas(t *testing.T) { | |||
| require.Equal(t, []byte("ok"), okValue) | |||
| } | |||
| // check block gas is always consumed | |||
| baseGas := uint64(35030) // baseGas is the gas consumed before tx msg | |||
| baseGas := uint64(40043) // baseGas is the gas consumed before tx msg | |||
There was a problem hiding this comment.
memo: Cheaper than the cosmos-sdk-v0.45.1
https://github.com/cosmos/cosmos-sdk/blame/v0.45.1/baseapp/block_gas_test.go#L126
| @@ -198,54 +196,6 @@ func (bva BaseVestingAccount) MarshalYAML() (interface{}, error) { | |||
| return marshalYaml(out) | |||
| } | |||
|
|
|||
| type vestingAccountJSON struct { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I left a few of these test cases because I thought it would be less likely to cause conflicts if only they were only test cases like this, but well, I'll remove them.
| @@ -804,87 +803,21 @@ func TestPermanentLockedAccountMarshal(t *testing.T) { | |||
| require.NotNil(t, err) | |||
| } | |||
|
|
|||
| func TestBaseVestingAccountMarshalJSONPB(t *testing.T) { | |||
| func TestBaseVestingAccountMarshal(t *testing.T) { | |||
There was a problem hiding this comment.
|
I think the gas consumption would have been reduced because 4 fields were merged into 1. Please check the gas comsumption value in the test codes. |
|
@tnasu All formatting differences from cosmos-sdk v0.45.1 in files under @dudong2 Based on preliminary findings, the Marshal Size in one field has increased by 35B (see also #549 (comment)). Perhaps the 4-fields version is stored in binary, whereas the 1-field version is stored in |
|
@torao I've confirmed
Got it. |
|
@tnasu I've made the changed files contained in those two PRs identical to the cosmos-sdk as much as possible within the scope of this issue. |
Description
This PR brings the performance fixes mainly done in 39afd48 back into the cosmos-sdk implementation based on the results of the performance comparison in #549.
BaseAccountProtocol Buffers definition have a single field for each type of public-key present.BaseAccountand revert to using ProtocolBuffers for serialization.closes: #549