Feat/ts v4.12.0 - #975
Open
gummy789j wants to merge 15 commits into
Open
Conversation
feat: add TRON governance command domains
…store import & backup
Review of d12bd1c (proposal / witness / contract governance, another contributor's work) against §1 and §2 of the v4.12.0 requirements, validated live on Nile with a registered witness account. Two defects made 6 of the 12 new commands unusable against a real node; both were invisible to the existing tests because those mock the gateway port, so they could only ever re-assert the adapter's own assumptions. - getWitness called /wallet/getwitnessbyaddress, which does not exist on any node (POST 405 / GET 404 on mainnet and Nile). Every witness-status check therefore failed with rpc_error, breaking `witness create`, `witness update`, `witness set-brokerage` and — via assertWitness — `proposal create`, `proposal approve` and `proposal delete`. Read the witness list and filter locally instead: one request, no fan-out, and listwitnesses covers every witness rather than only the active 27. - normalizeProposal rejected an array of parameters and fell back to {}, but listproposals only ever sends an array. Every proposal reported zero parameter changes — the field that says what a proposal does — and `proposal create --wait` could not resolve the id of the proposal it had just created, since findCreatedProposal matches on that set. Also gate the writes the Ledger TRON app cannot parse (WitnessCreate, WitnessUpdate, UpdateBrokerage, ClearABI, UpdateEnergyLimit, UpdateSetting): governanceTransactionMode already accepted requireSoftware but no call site passed it, so a Ledger user reached the device and spent RPCs before APDU 0x6a80. The proposal group stays ungated — its contract types are on the app's allowlist. See adr/0003. Tighten `witness create`'s activation check from "empty object" to a present address, matching accountExists, and make the fixtures realistic. Adds adapter-level coverage over a verbatim mainnet listproposals payload and per-command Ledger assertions; both fail if the fixes are reverted. Verified on Nile: witness update and set-brokerage confirmed on chain (url change re-read from listwitnesses); proposal create -> id 20662 resolved -> show renders the change -> approve -> already_approved -> --cancel -> not_approved -> delete -> canceled. Contract governance reaches real endpoints (not_contract_deployer / contract_not_found). create2 verified byte-exact against an independent implementation of Java's formula. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`contract set-origin-energy-limit` was rejected by every node with "Contract validate error : No contract!" — a message that points at the contract address, which was in fact correct. java-tron rebuilds the contract from the `raw_data` json on the non-visible broadcast path and IGNORES raw_data_hex. A numeric string does not parse into the int64 field, so the node validated an empty UpdateEnergyLimitContract, whose contract_address is empty. The CLI carries int64 quantities as strings by convention, and this builder passed that string straight into raw_data. Isolated on Nile with a single signed transaction, mutating only the json view so the signature and raw_data_hex stayed byte-identical: visible:false + hex + "9000000" -> CONTRACT_VALIDATE_ERROR, No contract! visible:false + hex + 9000000 -> accepted Coerced via #safeNumber, which refuses anything a json number cannot hold exactly. That replaces the previous "preserve a Java long" behaviour: such a value could only ever produce a transaction the node rejects, or silently set a rounded limit. Java's CLI can send int64 max because it speaks protobuf over gRPC; over HTTP+json we cannot, and refusing is the honest answer. Real limits are bounded by getTotalEnergyLimit (~1.8e11), far below the safe-integer ceiling, so nothing reachable is lost. Verified live on Nile after the fix: origin_energy_limit set to 12000000 and re-read from getcontract. The new test fails if the coercion is reverted. Note for follow-up: `proposal create` builds parameter values through the same local path and also stringifies values above 2^53, so it is likely to have the same defect. It is not reachable in practice — no TRON chain parameter is near that magnitude, and the actuator's own range checks reject absurd values — so it is reported rather than changed blindly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ites
All nine governance writes advertised --build-only and --sign-only and
neither worked: build-only failed outright with "this chain adapter cannot
produce transaction hex", and sign-only silently omitted `hex`, which is
the entire point of the flag. None of the three services passed the
pipeline's `artifact` hook.
Passing it alone would have been wrong. `artifact` serialises through
encodeTransactionHex, and the override table it consulted covered only the
TRC10 types — so ProposalCreate and UpdateEnergyLimit, the two types with
their own exact encoders precisely because tronweb encodes them wrongly,
would have produced hex from tronweb. That is worse than an honest error.
So the override dispatch is unified first: one table in transaction-codec
covering both families, consulted by BOTH protobuf paths. rawDataHexOf had
its own copy of the dispatch, which is how the two could disagree despite
toProtobuf's comment claiming otherwise. tx-integrity then drops its
per-type special-casing and simply compares against rawDataHexOf, removing
the TODO left when the branches were merged.
`artifact` only, deliberately not the full tronTransactionHooks: this group
binds --permission-id in each builder and applies --expiration via
withExtendedExpiration before the pipeline sees the transaction, so also
supplying `prepare` would rebind Permission_id and extend the expiration a
second time. Unifying on prepare is a separate refactor.
Verified end-to-end on Nile, for both custom-encoded types, through the
whole relay the flag exists for — build-only -> tx sign --hex -> tx
broadcast:
set-origin-energy-limit 146B unsigned -> 213B signed -> confirmed,
origin_energy_limit 15000000 read back on chain
proposal create 122B unsigned -> 189B signed -> confirmed as
proposal 20663 with its parameter change intact
(deleted afterwards)
sign-only now carries hex (184 bytes on witness set-brokerage).
Both new tests fail if their fix is reverted: the codec test asserts the
two protobuf paths agree and that the encoded value is not the placeholder
zero the exact encoders feed tronweb; the service test asserts all nine
writes pass the hook, including witness create, whose success path cannot
be exercised on chain.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The --records log filters are refused on an export path, but --offset slipped through: its .default(0) made "not given" indistinguishable from "given as 0" inside the refine, so `backup main --offset 5` was silently accepted and ignored. Make it optional, add it to RECORD_FILTERS, and skip its gap-fill prompt now that it has no default. The 0 already lives in backupRecords (query.offset ?? 0), so the emitted pagination is unchanged.
… value
`proposal list` and `proposal show` rendered every parameter as
`currentValue → proposedValue`, which reads as "this proposal changes it
from X to Y". Only the right-hand side comes from the proposal. The left
was fetched live from getChainParameters at render time, so for anything
past its voting window it was simply today's value, unrelated to the
baseline the proposal was created against — and for an approved proposal
it *is* the value that proposal installed, making the arrow read exactly
backwards.
Measured on Nile: of 21,739 parameter changes in settled proposals, 312
displayed a wrong old value, including 61 approved proposals shown as
`X → X`, i.e. a change that took effect rendered as a no-op.
The old value is not recoverable in general. `Proposal` carries only
proposal_id / proposer_address / parameters / expiration_time /
create_time / approvals / state; `parameters` is a map of parameter id to
target value and nothing records the prior one. It can be reconstructed by
replaying every approved proposal, but only for parameters some approved
proposal has already touched — 30% of historical changes bottom out at a
genesis default that later proposals have since overwritten, and that
default is unverifiable precisely where it is needed. So list/show now
report only what the proposal sets, per the v4.12.0 spec §1.1/§1.2, and
point at `chain params` for the values in effect now.
`proposal create` keeps the arrow (spec §1.3): there the baseline is read
while building the transaction, so it is provably the value being changed,
and it is what the user is deciding against.
Consequently list/show no longer call getChainParameters at all — names
and units come from the static definition table — so proposalParameters is
a pure function of the proposal. The list/show JSON follows the spec's
`parameters[]` of `{id, name, value, unit}` rather than `changes[]`, and
gets its own type so the dropped field cannot quietly return; 4.12.0 is
unreleased, so no consumer is broken. Text output adopts the spec's
right-aligned value column, and an empty list prints `(none)` like every
other list renderer instead of a bare header.
…repare hook a0ce486 closed with "Unifying on prepare is a separate refactor". This is that refactor, and it turns out three defects were downstream of the split. All nine governance writes advertised --permission-id and --expiration and neither worked in ANY execution mode. The builders bound both options themselves, so the services passed no `prepare` hook, and the pipeline reads "no prepare hook" as "this adapter cannot apply these options" — it threw invalid_option after the transaction had already been built. 9/9 commands, exit 2, including --dry-run and the default broadcast path. The multi-sig story for governance was simply unreachable. Routing the group through the shared tronTransactionHooks fixes that and removes the second expiration mechanism, which disagreed with the first in two ways: - Semantics. extendTransactionExpiration INCREMENTS raw_data.expiration; prepareTransaction SETS it to timestamp + N. So --expiration 60000 meant 120s here and 60s on tx send / contract send / contract deploy, and the governance value depended on whichever block timestamp the node served — the opposite of what an offline signing window needs. docs/commands/* already documented the absolute meaning, so the docs were right and the implementation was not. - Units. tronweb's extendExpiration takes SECONDS: it does parseInt(1e3 * extension) before adding. We passed milliseconds, so the window was multiplied by 1000 — --expiration 60000 would have produced 16.7 hours, and the documented 24h maximum 1000 days. This was unreachable only because the guard above rejected it first, which is why it had not been observed; fixing the guard alone would have converted a hard failure into a silent one. With prepare owning both options, extendTransactionExpiration and withExtendedExpiration have no callers and are removed along with the port declaration. Verified that prepare is lossless for the two custom-encoded governance types: toProtobuf consults the same override table, and with permissionId 0 and no expiration the re-derived bytes and txID are byte-identical to the builder's. The int64-boundary case still fails closed, now with "parameter value is outside int64" instead of tronweb's "Error generating a new transaction id." Two more defects in the same area: - Governance build-only/sign-only text output was built around `data.unsignedHex`, a field the pipeline never produces (it emits `hex`). Since kv() drops empty rows the artifact did not render blank, it vanished — the flag's entire purpose, available only via -o json. Print the bare hex as tx send does, so it pipes into a file or `tx sign`. - --permission-id was re-declared for governance with an int32 ceiling while transactionMode() accepts 0..9, so --help advertised a range the runtime refused, and the same mistake was classified invalid_option here but invalid_value everywhere else. TRON has at most eight active permissions (2..9) plus owner, so the override was simply wrong; dropping it inherits the correct bound. Also drops a dead `mode` in contract send: governanceTransactionMode's result was computed and discarded (the pipeline received transactionMode's), and both of its other effects were already performed on the two lines above. Tests drive the real TxPipeline. The existing governance suites pass a fake pipeline that records params, which is exactly why the guard never fired in test — only the real one has it. The pipeline's binding guard had no regression test at all; the one added here was verified to fail against a mutated guard rather than passing vacuously. Verified end-to-end against a controllable node: --permission-id 2 lands as Permission_id=2, --expiration 60000 yields exactly 60000ms on governance and on tx send, and build-only/sign-only write pipeable hex to stdout.
`asset info 1002438` reported a total supply of 666666666666666600. The node had said 666666666666666666. Sampling 1,400 mainnet assets: 252 carry a supply above 2^53, 10 rendered a wrong figure, and six of those printed 9223372036854776000 — larger than int64 permits, so a value the chain cannot hold and no asset can have. total_supply and frozen_amount are protocol int64. These endpoints were the last ones still reaching the node through tronweb, whose HTTP provider parses the body with a plain JSON.parse: the value was already a rounded float64 before this class saw it, so the String() the service applies downstream can only stringify damage that has already happened. The port then declared that damage (`total_supply: number`), which is why the compiler had nothing to say. The repository already solved this — getAccount, getBlock, listwitnesses and listproposals fetch and run parseLosslessJson. The TRC10 and exchange reads simply never followed. This brings the remaining seven into line: /wallet/getassetissuebyid /wallet/getexchangebyid /wallet/getassetissuebyaccount /wallet/getpaginatedexchangelist /wallet/getassetissuelistbyname /wallet/gettransactioninfobyid /wallet/getpaginatedassetissuelist tronweb is otherwise untouched — it still builds every transaction, converts addresses and serialises protobuf. What it also did for these calls, we now do: decoding the hex text fields and handling each response shape. Those shapes were checked against mainnet rather than assumed, which is what the new tests encode. The read and write models of a frozen tranche split, because they genuinely differ. Reading carries the int64 exactly, as a decimal string. Issuance keeps `number`: java-tron rebuilds the contract from raw_data json on the non-visible broadcast path, and a numeric *string* does not parse into an int64 field there. So this client can now report a supply above 2^53 correctly while still refusing to issue one — an asymmetry that is the protocol's, and is now deliberate rather than accidental. Realised amounts in a --wait receipt (unfreeze_amount, withdraw_amount and the three exchange_* quantities) become exact for the same reason: on a high-supply TRC10 they exceed 2^53. Fees and resource counters deliberately stay numbers — 2^53 sun is nine billion TRX, so widening them would change the machine contract of every --wait receipt and buy nothing. Exchange reserves are included even though no mainnet pair is near the boundary today (0 of 252 balances sampled). They are the one quantity here that is not merely displayed: proportionalOther and bancorOutput compute the amounts that get signed into inject/withdraw/trade from them. The CLI's json contract is unchanged throughout: totalSupply was already emitted as a string. Only the value is now the one the chain reports. before: "totalSupply": "666666666666666600" after: "totalSupply": "666666666666666666"
…otocol range
Ask a node for TRC10 1234 and it could answer with asset 9999 — different
issuer, different rate, different ICO window — and nothing noticed. The same
held for exchange pools: `#requireExchange` checked only that a record came
back, then inject/withdraw/trade built against whatever id it carried.
Response identity is the one claim about a by-id lookup that can be verified
without trusting the node, and it was not being verified.
Alongside it, two fields are constrained by the contract rather than by
observation: `precision` is 0..6, and the `trx_num`/`num` rate pair is a
positive int32. Reading every TRC10 on mainnet (5,192) and Nile (4,000)
found zero records violating either — so refusing one is not a compatibility
risk, it is a broken or dishonest node. Absence stays legal: 47.67% of
mainnet assets omit `precision` entirely, which means 0, and the checks
treat it that way.
This closes three concrete failures, all previously reachable with a node
that answers oddly:
- an out-of-range `precision` reached fromBaseUnits, whose padStart then
allocated a string proportional to it;
- a zero/negative rate pair reached icoPriceLabel, which surfaced as
`internal_error` — a code that blames this client for the node's answer;
- a by-id lookup could be answered with a different object entirely.
It does NOT close the case the identity check is often assumed to: a node
reporting a precision that is wrong but in range. Measured, with only the
node's answer varying:
tx send --asset-id 1000001 --amount 1
node says precision=6 -> amount 1000000 signed
node says precision=0 -> amount 1 signed
Both values are legal, so no local rule rejects either, and the factor of a
million survives this commit untouched. There is no invariant here to check
against — unlike an id, a precision cannot be compared to anything we already
know. Mitigating it means showing the raw amount before signing, or telling
users to pass --raw-amount when it must be certain; neither belongs in this
change, and neither should be assumed to be in place because identity now is.
`getTrc10Info` is included, which matters more than the rest: `tx send
--asset-id` reads its decimals there rather than from getAssetById, so it is
the TRC10 signing path most users take and it would otherwise have kept the
hole its neighbours just closed.
Single-record reads throw; list reads drop the offending row and return the
page. A list is a display surface and one poisoned record should not deny the
caller the other 199 — but a name lookup filters too, because its single
match can still reach a signing path.
`invalid_node_response` is documented in machine-interface.md rather than
left for the next audit to find, which is how the two documentation defects
in this same release got there.
…ght one
Four unrelated defects, each one where the code produced something plausible
instead of stopping.
**A keystore could authenticate any password.** The Web3 MAC is
keccak(dk[16:32] || ciphertext), so it binds to the password only through
that slice. A file declaring `dklen: 16` leaves the slice EMPTY and the MAC
collapses to keccak(ciphertext) — a constant its author picks. Every
password then passes, decrypts to different 32 random bytes, and that is a
well-formed private key: import reports success and the user holds an
address nobody knows the key to. Reproduced with three different passwords
against one file, yielding three different addresses, on both accepted KDFs.
Refusing dklen < 32 is not a divergence from the Java implementation this
codec claims parity with — it is catching up. Java's pbkdf2 path ignores
`dklen` and always derives 32 bytes; its scrypt path would throw copying a
16-byte slice. Both are safe; we were not. The header comment promising
"anything it can open, we can open" now records the exception.
**The export audit log destroyed itself on schema drift.** An append is a
whole-file rewrite, and any legal json that was not the shape we expected
read as "no records" — so the next export silently overwrote it. The likely
trigger is not tampering but us: a future build writing `version: 2`, opened
once by this one, loses the history. Unreadable content is now set aside as
`.unreadable-<n>` before anything is written. Export keeps working, because
refusing to back up when the log is unreadable helps nobody, but the
original bytes survive. Syntactically broken json already failed closed and
still does; that asymmetry was the tell.
The test that pinned the old behaviour asserted the data loss as a feature
("treats a file with no usable records array as empty"), so it is replaced
rather than extended.
**A committed secret could go unreported.** Backup writes the file, then
records the export. If the second step throws, the first has already
happened — and the command reported a plain failure, so the caller learned
neither that a secret existed nor where. Without `--out` that is a
timestamped file in the process's working directory, which nobody can guess,
and retrying writes a second copy. It stays a failure, because the export
did not fully succeed, but `audit_append_failed` now carries `details.out`
so the file can be found and shredded.
**An impossible ICO time became a different, valid one.** The parser took
any two digits per component, let `Date.UTC` roll them over, then checked
only the resulting DATE — which catches `24:00:00` (it lands on the next
day) but not `12:60:00`, silently read as 13:00:00. Issuance burns a fee,
an account may only ever issue once, and the window is fixed for the life of
the token, so comparing the whole instant is the least this deserves.
Documentation, in the same spirit of not implying more certainty than exists:
- `--amount` is scaled by decimals the NODE supplies. The range checks added
earlier reject a TRC10 precision outside 0..6 and a record answering for
another id, but a wrong value INSIDE that range cannot be detected locally
— there is nothing to compare it against. `tx send` and the exchange guide
now say so, and point at the `--raw-*` flags for when it must be exact.
- `contract deploy --build-only` documented `unsigned`/`unsignedHex`; the
producer emits `tx`/`hex`, so anything reading the documented names got
undefined.
- The pagination inventory listed `proposal show`, which returns one
proposal and no cursor, and omitted `proposal list`, which does paginate.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.