Use integer for root intra in TxnExtra
#763
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #763 +/- ##
===========================================
- Coverage 55.20% 55.09% -0.11%
===========================================
Files 26 26
Lines 3817 3828 +11
===========================================
+ Hits 2107 2109 +2
- Misses 1435 1443 +8
- Partials 275 276 +1
Continue to review full report at Codecov.
|
|
Our current codec without the The only extra code here is the marshal/unmarshal functions plus tests. I think this is worth having to isolate encoding and logic. |
| } | ||
|
|
||
| // UnmarshalText implements TextUnmarshaler interface. | ||
| func (ou *OptionalUint) UnmarshalText(text []byte) error { |
There was a problem hiding this comment.
This feels more like a utility function that should live independently of idb and have its own testing.
There was a problem hiding this comment.
This is a method on type OptionalUint, so we can't separate them. Do you think I should move the type elsewhere?
There was a problem hiding this comment.
Sorry, I was referring to the overall OptionalUint type as being general purpose - though we should have at least 1 other use-case before moving it (I'm used to optionals etc from functional languages).
idb/idb.go
Outdated
|
|
||
| return nil | ||
| } | ||
|
|
||
| // TxnExtra is some additional metadata needed for a transaction. | ||
| type TxnExtra struct { | ||
| AssetCloseAmount uint64 `codec:"aca,omitempty"` | ||
| // RootIntra is set on inner transactions. Combined with the confirmation | ||
| // round it can be used to lookup the root transaction. | ||
| // The type is string to allow distinguishing between 0 and empty. |
There was a problem hiding this comment.
Update the comment to match code changes.
This change is too separate encoding from the business logic. As you can see, before we were doing integer <-> string conversions in the business logic. Functionally, this PR doesn't change anything. |
|
More concretely, the json codec will invoke |
| // round it can be used to lookup the root transaction. | ||
| // The type is string to allow distinguishing between 0 and empty. | ||
| RootIntra string `codec:"root-intra,omitempty"` |
There was a problem hiding this comment.
It just occurred to me that we can use RootTxid to determine if this field exists or not. We can avoid special encoding/decode code entirely and use a uint64 type here and use len(RootTxid) > 0 as the "Present" condition.
From there we just need to make sure RootIntra has a comment saying "0 is a valid value when RootTxid is set."
There was a problem hiding this comment.
The query would need to change though to handle the special case when t.extra->>'root-intra' is empty string. https://github.com/algorand/indexer/pull/721/files#diff-8fbec00be6e6b2f77a294cbe9a3ee8e3fbdcb5eb13be3046e5da8277302264c2R637
If you would like to change it, I will not object.
By the way, I'm afraid this query has quadratic complexity. I'm not sure postgres is smart enough to look up the right root txn with one query. Possibly, for each txn row there will be a scan of all rows for this round.
tolikzinovyev
force-pushed
the
tolik/root-intra-encoding
branch
from
16e06bf
to
1d1565c
Compare
Summary
Found a way use integer for root intra. This doesn't change the format of json.
Test Plan
Added usual encoding tests.