util: JSONify uint160 using LE instead of BE #785

AnnaShaleva · 2020-03-23T09:53:14Z

closes #769

codecov · 2020-03-23T09:55:02Z

Codecov Report

Merging #785 into master will decrease coverage by 0.00%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #785      +/-   ##
==========================================
- Coverage   64.34%   64.34%   -0.01%     
==========================================
  Files         141      141              
  Lines       12934    12916      -18     
==========================================
- Hits         8323     8311      -12     
+ Misses       4208     4203       -5     
+ Partials      403      402       -1

Impacted Files	Coverage Δ
pkg/rpc/response/result/account_state.go	`0.00% <0.00%> (ø)`
pkg/rpc/response/result/asset_state.go	`0.00% <ø> (ø)`
pkg/rpc/response/result/contract_state.go	`0.00% <0.00%> (ø)`
pkg/smartcontract/parameter.go	`75.79% <100.00%> (+0.19%)`	⬆️
pkg/util/uint160.go	`100.00% <100.00%> (ø)`
pkg/wallet/token.go	`100.00% <100.00%> (+8.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b37c593...2187346. Read the comment docs.

roman-khimov · 2020-03-23T09:59:04Z

pkg/rpc/response/result/asset_state.go

+		Available:  a.Available,
+		Precision:  a.Precision,
+		FeeMode:    a.FeeMode,
+		FeeAddress: a.FeeAddress.Reverse(), // FeeAddress should be marshaled in BE but default marshaler uses LE.


Are you sure about that?

It looks to me that it's always zero and there are no examples that could prove it being LE or BE in real life. Thus, I'd suggest using the default (which is LE) and not adding yet another wrapper.

Actually, I'm not sure because official RPC API (https://docs.neo.org/docs/en-us/reference/rpc/latest-version/api/getassetstate.html) does not suppose serialization of fields FeeAddress and FeeMode (see also https://github.com/neo-project/neo/blob/master-2.x/neo/Ledger/AssetState.cs#L151).
In neo-go these fields are serialized, particularly, FeeAddress was serialized in BE, so I decided to leave it as it was before.

Also fields ID and AssetType of AssetState JSONify as assetID and assetType respectively, but in C# API it should be just id and type. Should I fix it?

It looks to me that it's always zero and there are no examples that could prove it being LE or BE in real life. Thus, I'd suggest using the default (which is LE) and not adding yet another wrapper.

Fixed

official RPC API does not suppose serialization of fields FeeAddress and FeeMode

We can drop them from the answer then.

Should I fix it?

Yep, we should be 100% compatible in our outputs.

roman-khimov · 2020-03-23T10:06:45Z

pkg/wallet/token.go

@@ -38,7 +38,7 @@ func NewToken(tokenHash util.Uint160, name, symbol string, decimals int64) *Toke
 func (t *Token) MarshalJSON() ([]byte, error) {
 	m := &tokenAux{


Maybe we don't need this struct at all now?

We use this struct in UnmarshalJSON too because it does not contains Address and we have to fill this field. Unmarshalling into &tokenAux lets us avoid recursion, so perhaps we still need it?

json:"-"? And maybe this structure shouldn't have an Address field, but rather an Address() method (and maybe address field as a cache, though I doubt it's worth it, this structure is never in the hot path).

Done (added Address() method)

roman-khimov · 2020-03-23T10:11:48Z

pkg/smartcontract/context/item.go

@@ -36,7 +36,7 @@ func (it *Item) AddSignature(pub *keys.PublicKey, sig []byte) {
 // MarshalJSON implements json.Marshaler interface.
 func (it Item) MarshalJSON() ([]byte, error) {
 	ci := itemAux{
-		Script:     it.Script,
+		Script:     it.Script.Reverse(), // script should be marshaled in BE but default marshaler uses LE.


It'd be nice to check this against some C# examples.

They store Script as []byte in LE, so fixed

roman-khimov · 2020-03-23T10:34:26Z

pkg/smartcontract/parameter.go

@@ -197,7 +199,7 @@ func (p *Parameter) UnmarshalJSON(data []byte) (err error) {
 		if err = json.Unmarshal(r.Value, &h); err != nil {
 			return
 		}
-		p.Value = h
+		p.Value = h.Reverse() // Hash160 should be marshaled in BE but default marshaler uses LE.


Not sure about this also.

For UInt160 here (https://github.com/neo-project/neo/blob/master-2.x/neo/SmartContract/ContractParameter.cs#L156) they use override ToString() method (https://github.com/neo-project/neo/blob/master-2.x/neo/UIntBase.cs#L126) which returns BE string

But our internal storage is BE and theirs is LE IIRC, so reversing here gives you LE.

AnnaShaleva · 2020-03-23T13:03:44Z

pkg/util/uint160.go

@@ -123,11 +123,11 @@ func (u *Uint160) UnmarshalJSON(data []byte) (err error) {
 		return err
 	}
 	js = strings.TrimPrefix(js, "0x")
-	*u, err = Uint160DecodeStringBE(js)
+	*u, err = Uint160DecodeStringLE(js)


@roman-khimov Which one should we use here?

LE, that's the whole point of #769.

roman-khimov · 2020-03-23T14:13:10Z

pkg/rpc/response/result/asset_state.go

-	FeeMode    uint8                 `json:"fee"`
-	FeeAddress util.Uint160          `json:"address"`
+	FeeMode    uint8                 `json:"-"`
+	FeeAddress util.Uint160          `json:"-"`


Why not just drop them?

And make a separate commit, because it's a separate change.

closes #769

Modified result.AssetState: - removed `FeeMode` and `FeeAddress` fields - fixed json name of `ID` and `AssetType` fields to be consistent with C# RPC server

There's a bug after #785: smartcontract.Parameter of type hash160 should be marshalled in LE (as default marshaller for uint160 does) instead of BE, so fixed.

AnnaShaleva added this to the v0.74.1 milestone Mar 23, 2020

AnnaShaleva requested a review from roman-khimov March 23, 2020 09:53

AnnaShaleva self-assigned this Mar 23, 2020

roman-khimov reviewed Mar 23, 2020

View reviewed changes

AnnaShaleva force-pushed the feature/uint160_marshalling branch from a45a2ce to 7305750 Compare March 23, 2020 10:22

roman-khimov requested changes Mar 23, 2020

View reviewed changes

AnnaShaleva force-pushed the feature/uint160_marshalling branch 5 times, most recently from 79f0170 to cac366f Compare March 23, 2020 12:57

AnnaShaleva commented Mar 23, 2020

View reviewed changes

roman-khimov reviewed Mar 23, 2020

View reviewed changes

AnnaShaleva added 2 commits March 23, 2020 17:38

util: JSONify uint160 using LE instead of BE

2001a40

closes #769

rpc: fix json marshalling of result.AssetState

2187346

Modified result.AssetState: - removed `FeeMode` and `FeeAddress` fields - fixed json name of `ID` and `AssetType` fields to be consistent with C# RPC server

AnnaShaleva force-pushed the feature/uint160_marshalling branch from cac366f to 2187346 Compare March 23, 2020 14:48

roman-khimov approved these changes Mar 24, 2020

View reviewed changes

roman-khimov merged commit 751e79d into master Mar 24, 2020

roman-khimov deleted the feature/uint160_marshalling branch March 24, 2020 09:41

AnnaShaleva mentioned this pull request Apr 1, 2020

smartcontract: fix uint160 marshalling in smartcontract.Parameter #819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: JSONify uint160 using LE instead of BE #785

util: JSONify uint160 using LE instead of BE #785

AnnaShaleva commented Mar 23, 2020

codecov bot commented Mar 23, 2020 •

edited

Loading

roman-khimov Mar 23, 2020

roman-khimov Mar 23, 2020

AnnaShaleva Mar 23, 2020

AnnaShaleva Mar 23, 2020 •

edited

Loading

AnnaShaleva Mar 23, 2020

roman-khimov Mar 23, 2020

AnnaShaleva Mar 23, 2020

roman-khimov Mar 23, 2020

AnnaShaleva Mar 23, 2020

roman-khimov Mar 23, 2020

AnnaShaleva Mar 23, 2020

roman-khimov Mar 23, 2020

AnnaShaleva Mar 23, 2020

roman-khimov Mar 23, 2020

AnnaShaleva Mar 23, 2020

roman-khimov Mar 23, 2020

AnnaShaleva Mar 23, 2020

roman-khimov Mar 23, 2020

roman-khimov Mar 23, 2020

roman-khimov Mar 23, 2020

AnnaShaleva Mar 23, 2020

		@@ -38,7 +38,7 @@ func NewToken(tokenHash util.Uint160, name, symbol string, decimals int64) *Toke
		func (t *Token) MarshalJSON() ([]byte, error) {
		m := &tokenAux{

util: JSONify uint160 using LE instead of BE #785

util: JSONify uint160 using LE instead of BE #785

Conversation

AnnaShaleva commented Mar 23, 2020

codecov bot commented Mar 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2020 •

edited

Loading

AnnaShaleva Mar 23, 2020 •

edited

Loading