qkc/types: add token balances by blockchaindevsh · Pull Request #33 · QuarkChain/goshard

blockchaindevsh · 2026-07-11T15:10:40Z

qzhodl · 2026-07-14T09:06:31Z

+
+type TokenBalances struct {
+	db        *triedb.Database
+	tokenTrie *trie.SecureTrie


As we discussed in a previous meeting, since there is only one MNT token on the QuarkChain mainnet (<16) and users cannot create new ones, we can remove the tokenTrie and use only balances.

You can also refer to Ping's design doc and PR for the MNT implementation.

Fixed in ce7e4f5 .

qzhodl

Both inline issues below were found during the first-pass Codex scan of this PR. Was an AI-assisted review run before the code was submitted?

qzhodl · 2026-07-15T10:09:16Z

+func (t *TokenBalances) MarshalJSON() ([]byte, error) {
+	balances := ""
+	for key, val := range t.balances {
+		bal := fmt.Sprintf("%d:%d", key, val.Uint64())


[P2] Preserve the full uint256 value in JSON

Balance supports 256-bit values, but Uint64() keeps only the low 64 bits. For example, a balance of 2^80 is emitted as 0, so a JSON round trip silently changes the balance. The inverse path has the same range problem because it scans into Go int and calls SetUint64. Please format the full decimal value (for example, with val.Dec() or ToBig().String()), parse it into uint256.Int, parse token IDs with strconv.ParseUint, and add a round-trip case above math.MaxUint64.

Fixed in 81fa2ea.

qzhodl · 2026-07-15T10:09:16Z

+	mapping[3] = testU256(99)
+
+	b := NewTokenBalancesWithMap(mapping)
+	encoded, err := rlp.EncodeToBytes(b)


[P2] Add an independent Python golden for the QKC serializer

These tests cover (*TokenBalances).SerializeToBytes (the 0x00 || RLP(...) state encoding) and the RLP codec, but they do not exercise the distinct QKC serialization path serialize.SerializeToBytes(tb), which dispatches to TokenBalances.Serialize, or the corresponding Deserialize path. Even a Go-only encode/decode round trip would be insufficient, because the Go encoder and decoder could share the same incorrect assumption while still disagreeing with pyquarkchain. Please compare both encoding and decoding against independently generated pyquarkchain bytes. For example, TokenBalanceMap({3232: 109, 0: 0, 3567: 999999}).serialize().hex() produces 00000002020ca0016d020def030f423f in the current Python reference.

Fixed in b6b6b79.

blockchaindevsh · 2026-07-15T12:49:17Z

Both inline issues below were found during the first-pass Codex scan of this PR. Was an AI-assisted review run before the code was submitted?

Answer from codex:

I found it. The last prompt where you asked me to review this set of code was:
“The design doc looks okay now. Next, review the 5 sub-PRs to see whether their contents are necessary, whether they are minimal, and whether there are any redundant changes: PR #25, #33, #34, #35, #36.”
At that time, my review did say that PR #33 was basically minimal, but I did not go deep enough into the serializer/JSON details added after ce7e4f5. Later, when we moved into the “address comment” flow, I focused only on the reviewer’s comment and did not re-review the PR from the broader angle of “are there other issues this PR should have caught earlier?”
Going forward, I’ll adjust the process:
When you ask me to review a PR, I’ll first do a full review: minimality, necessity, tests, and potential issues.
When a reviewer later leaves a comment, I’ll:
fix only the current comment, to avoid mixing issues together;
also re-review the affected file to check whether there are any issues I previously missed, instead of mechanically applying just the requested change.
That should prevent cases like this one, where the reviewer had to point out the JSON and Python golden issues one after another, while I should have caught them in the initial review.

qzhodl · 2026-07-16T02:55:48Z

+		if err := serialize.Deserialize(bb, v); err != nil {
+			return err
+		}
+		if v.Cmp(common.Big0) == 0 {


[P2] Preserve Python last-write-wins semantics for duplicate token IDs

PrependedSizeMapSerializer.deserialize first assigns every decoded pair into a dictionary and only then applies the zero-value filter. For example, 0000000201010101010100 encodes two entries, (tokenID=1, balance=1) followed by (tokenID=1, balance=0). Python decodes this as an empty map and reserializes it as 00000000. This implementation skips the final zero before it can overwrite or delete the earlier value, so Go retains {1: 1} and reserializes it as 0000000101010101. The same QKC wire bytes therefore produce different logical coinbase maps and can affect header hashing or validation. Please apply last-write-wins before filtering zeros, or at minimum delete a previously decoded entry when the final value for that key is zero, and add this Python-generated decode vector as a regression test.

Fixed in f569468.

qzhodl · 2026-07-16T02:55:48Z

+			Balance: v,
+		})
+	}
+	if len(list) == 0 {


[P2] Preserve the Python zero-only account-state encoding

Since account-state encoding is intentionally retained in this type, a non-empty balance cache containing only zero values must remain byte-compatible with evm.state.TokenBalances. Python checks whether _balances is empty before filtering zero values, so an in-memory map such as {1: 0}—which is reachable through set_balance(..., 0)—serializes as 00c0 (0x00 || RLP([])). This post-filter empty check instead returns no bytes for the same map; the outer RLP is consequently 80 rather than 8200c0, which can change account RLP and the state root. Please keep the true empty-map encoding as empty, but encode a non-empty zero-only map as 00c0, and add a Python golden for both the inner bytes and the outer RLP.

Fixed in 63c240d.

Just check goquarkchain, and it do not have this issue.

qzhodl · 2026-07-16T03:07:34Z

+			continue
+		}
+		if data, ok := t.balances[k]; ok {
+			t.balances[k] = new(uint256.Int).Add(v, data)


[P2] Do not silently wrap token balance additions

uint256.Int.Add computes modulo 2^256. Starting with balance 2^256 - 1 and adding 1 therefore changes this token to zero; both serialization paths then omit the token entirely. The Python reference uses arbitrary-precision addition, so the same operation produces 2^256 (and TokenBalanceMap can encode that 33-byte biguint). Even if this Go type intentionally restricts its internal representation to uint256, an overflow should be detected instead of silently changing a reward/fee total into zero. Please use AddOverflow and propagate an error (or otherwise enforce the invariant explicitly), and add a max-plus-one regression test.

Fixed in 0054718.

ping-ke · 2026-07-16T02:15:31Z

+	"github.com/holiman/uint256"
+)
+
+type TokenBalancePair struct {


we can add some comment as we remove "> 16 tokens",
// TokenBalances holds multi-token balances for an account.
// Serialization uses list format (0x00 prefix) for ≤ TokenTrieThreshold non-zero
// balances. Trie format (> 16 tokens) is not supported; SerializeToBytes returns
// an error if more than TokenTrieThreshold non-zero balances are present.

You can referance this code https://github.com/QuarkChain/goshard/blob/3860cb77f14676120d66637ab5fe692fc1cfe0bb/qkc/common/token.go

I made a change so that it'll fail loudly if > 16 tokens in 738ab78, @qzhodl may also take a look.

ping-ke · 2026-07-16T02:22:02Z

+	return tokenBalances, nil
+}
+
+func (t *TokenBalances) Commit() {


I think we need to remove this func or panic to avoid misuse.

Fixed in dec8117.

ping-ke · 2026-07-16T08:08:08Z

-}
-
-func (t *TokenBalances) Add(other map[uint64]*uint256.Int) {
+func (t *TokenBalances) Add(other map[uint64]*uint256.Int) error {


This may need to change, as big.Int has minus value, while uint256.Int not. so this can only increase.

Add only supports non-negative deltas — the other values are *uint256.Int,
which can't represent a negative amount, and the overflow guard rejects wraps.
pyquarkchain's TokenBalanceMap.add allows negative v:

self.balance_map[k] = self.balance_map.get(k, 0) + v

which is used for cross-shard debits. This is fine for merge/credit-only use, but
the semantics are narrower than the original. No callers exist yet, so flagging
for when this gets wired up: if a debit path is ever needed, this signature
(unsigned map, no subtraction) won't cover it.

Fixed in a2fc851.

ping-ke · 2026-07-16T09:05:07Z

-}
-
-func (t *TokenBalances) Add(other map[uint64]*uint256.Int) {
+func (t *TokenBalances) Add(other map[uint64]*uint256.Int) error {


Add only supports non-negative deltas — the other values are *uint256.Int,
which can't represent a negative amount, and the overflow guard rejects wraps.
pyquarkchain's TokenBalanceMap.add allows negative v:

self.balance_map[k] = self.balance_map.get(k, 0) + v

which is used for cross-shard debits. This is fine for merge/credit-only use, but
the semantics are narrower than the original. No callers exist yet, so flagging
for when this gets wired up: if a debit path is ever needed, this signature
(unsigned map, no subtraction) won't cover it.

ping-ke · 2026-07-16T09:05:25Z

+}
+
+func (t *TokenBalances) SetValue(amount *uint256.Int, tokenID uint64) {
+	if amount == nil {


SetValue stores zero-valued entries, which faithfully mirrors pyquarkchain's
TokenBalances.set_balance (it also stores zeros in _balances). However, in
pyquarkchain every output path drops zero entries:

serialize(): if v > 0 filter

is_blank(): counts only non-zero entries

to_dict(): if v == 0: ret.pop(k)

commit(): if bal: update else delete

So storing a zero is never externally observable. SerializeToBytes and
IsBlank here already filter correctly, but GetBalanceMap() and
MarshalJSON() do NOT — they leak zero entries that pyquarkchain's to_dict()
would strip.

Suggestion: drop zeros on write instead (if amount == nil || amount.IsZero() { delete(t.balances, tokenID); return }), matching qkc/common/token.go. This is
functionally equivalent to pyquarkchain (all outputs filter zeros anyway) and
removes the Get/Marshal divergence for free, rather than filtering in three
separate output methods.

I don’t think we should drop zeros on write here.

In pyquarkchain, set_balance() does store zero values in _balances, and the zero-only in-memory state serializes as 0x00 || rlp([]) (00c0) rather than empty bytes. This behavior is also covered by pyquarkchain’s test_encode_zero_balance.

So changing SetValue(0) to delete the entry would change SerializeToBytes() / EncodeRLP() output for zero-only state and could affect state/account hashing. I’ll keep the current behavior for account-state compatibility. JSON/GetBalanceMap can be revisited separately if we later split this from the account-state type.

qkc/types: add token balances

d78aaa3

blockchaindevsh requested review from iteyelmp, ping-ke, qizhou, qzhodl and syntrust July 11, 2026 15:10

blockchaindevsh mentioned this pull request Jul 13, 2026

EthStorage Devs Meeting #185 Agenda ethstorage/pm#267

Closed

qzhodl reviewed Jul 14, 2026

View reviewed changes

qkc/types: remove token balance trie state

ce7e4f5

qzhodl reviewed Jul 15, 2026

View reviewed changes

blockchaindevsh added 2 commits July 15, 2026 20:38

qkc/types: preserve full token balance json

81fa2ea

qkc/types: add token balance serializer golden

b6b6b79

qzhodl reviewed Jul 16, 2026

View reviewed changes

ping-ke reviewed Jul 16, 2026

View reviewed changes

blockchaindevsh added 5 commits July 16, 2026 12:26

qkc/types: preserve duplicate token decode semantics

f569468

qkc/types: preserve zero-only token encoding

63c240d

qkc/types: reject token balance addition overflow

0054718

qkc/types: limit token balance list size

738ab78

qkc/types: remove token balances commit hook

dec8117

ping-ke reviewed Jul 16, 2026

View reviewed changes

qzhodl approved these changes Jul 16, 2026

View reviewed changes

ping-ke reviewed Jul 16, 2026

View reviewed changes

qkc/types: support token balance deltas

a2fc851

blockchaindevsh mentioned this pull request Jul 16, 2026

QKC/OP Devs Meeting #103 QuarkChain/pm#158

Closed

Uh oh!

Conversation

blockchaindevsh commented Jul 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qzhodl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qzhodl Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blockchaindevsh Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blockchaindevsh commented Jul 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

qzhodl left a comment •

edited

Loading

qzhodl Jul 15, 2026 •

edited

Loading

blockchaindevsh Jul 15, 2026 •

edited

Loading