release v1.1.0 - #83
Conversation
Will-Guan
left a comment
Will-Guan
left a comment
There was a problem hiding this comment.
Finding
- Medium — Parsed token symbols can be silently ignored.
The TRON batch, exact, and upto price-parsing paths can discard the symbol returned byparseMoney(), causing prices such as"1 USDD"or"$1 USDD"to fall back to the network default USDT asset. Preserve the symbol during token resolution or explicitly reject unsupported suffixes.
Suggestion
- Enforce a consistent zero-amount policy.
Core and EVM intentionally truncate sub-atomic prices to"0", while TRON exact and upto reject them locally. If paid routes must charge at least one atomic unit, validateBigInt(parsedPrice.amount) > 0ncentrally when buildingPaymentRequirements, rather than adding a TRON-batch-only guard.
| @@ -240,7 +243,7 @@ export class BatchSettlementTronScheme implements SchemeNetworkServer { | |||
| return { amount: price.amount, asset: price.asset, extra: price.extra || {} }; | |||
| } | |||
|
|
|||
| const amount = this.parseMoneyToDecimal(price); | |||
| const amount = parseMoney(price).amount; | |||
There was a problem hiding this comment.
Medium · bug — Preserve or reject the parsed token symbol
parseMoney() returns { amount, symbol }, but this line keeps only amount, so the fallback conversion always uses the network default asset. For example, parsePrice("1 USDD", Nile) is silently converted to 1_000_000 units of Nile USDT instead of USDD. Unsupported symbols can likewise be treated as USDT without an error.
The EVM implementation preserves symbol and passes it to asset resolution. Please either propagate the symbol into the TRON token lookup/conversion path or explicitly reject non-USD suffixes that this path does not support.
| @@ -76,7 +79,7 @@ export class ExactTronScheme implements SchemeNetworkServer { | |||
| } | |||
|
|
|||
| // Parse Money to decimal number | |||
| const amount = this.parseMoneyToDecimal(price); | |||
| const amount = parseMoney(price).amount; | |||
There was a problem hiding this comment.
Same finding — the token symbol is lost on the dollar-prefixed path
A value such as "$1 USDD" does not match the token-price branch above, because that expression does not accept the $ prefix. It therefore reaches parseMoney(), where USDD is parsed successfully but discarded by selecting only .amount, and the price falls back to the default USDT asset.
Please preserve and resolve the returned symbol, or explicitly reject this syntax instead of silently changing the requested asset.
| @@ -61,7 +63,7 @@ export class UptoTronScheme implements SchemeNetworkServer { | |||
| return parseTokenPrice(price.trim(), network); | |||
| } | |||
|
|
|||
| const amount = this.parseMoneyToDecimal(price); | |||
| const amount = parseMoney(price).amount; | |||
There was a problem hiding this comment.
Same finding — the token symbol is lost on the dollar-prefixed path
As in the exact implementation, "$1 USDD" bypasses the token-price branch, after which parseMoney() extracts USDD but this line discards it. The resulting requirement is then denominated in the default USDT asset.
Please handle the parsed symbol consistently with the token-price path, or reject unsupported suffixes explicitly.
Will-Guan
left a comment
Will-Guan
left a comment
There was a problem hiding this comment.
The parseMoney migration now preserves explicit token symbols across all three TRON schemes. Two consistency issues remain:
- Exact and Upto retain the old
<amount> <symbol>regex path beforeparseMoney. Besides duplicating the new symbol handling, this makes1 USDfail as an unknown token while$1 USDand Batch Settlement use the default asset. - Positive sub-atomic amounts still follow different policies: Exact and Upto reject them locally, while Batch Settlement truncates them to zero. This should follow one centrally enforced policy, consistent with the earlier review discussion.
I suggest using the single parseMoney -> symbol -> token registry flow in all three schemes and adding parameterized coverage for 1 USD, $1 USD, and positive sub-atomic amounts. The stale parseMoneyToDecimal JSDoc blocks left in the three files can also be removed.
| @@ -76,7 +79,10 @@ export class ExactTronScheme implements SchemeNetworkServer { | |||
| } | |||
|
|
|||
| // Parse Money to decimal number | |||
| const amount = this.parseMoneyToDecimal(price); | |||
| const { amount, symbol } = parseMoney(price); | |||
There was a problem hiding this comment.
Medium · bug — Use one parsing path for token-symbol prices
Could we remove the preceding regex fast path now that parseMoney returns the symbol? Both paths produce the same result for 1 USDT, but the regex bypasses the explicit USD handling in parseMoney. As a result, 1 USD throws Unknown token, while $1 USD correctly uses the default asset. Batch Settlement already uses the unified path, so following the same flow here would remove the duplication and restore consistent semantics.
| @@ -61,7 +63,10 @@ export class UptoTronScheme implements SchemeNetworkServer { | |||
| return parseTokenPrice(price.trim(), network); | |||
| } | |||
|
|
|||
| const amount = this.parseMoneyToDecimal(price); | |||
| const { amount, symbol } = parseMoney(price); | |||
There was a problem hiding this comment.
Medium · bug — Use one parsing path for token-symbol prices
Could we remove the preceding <amount> <symbol> regex branch and let parseMoney handle all Money inputs? The current dual paths make 1 USD fail as an unknown token even though parseMoney intentionally treats USD as ordinary money, while $1 USD succeeds. This would also align Upto with the single parsing path already used by Batch Settlement.
| const assetInfo = getDefaultAsset(network); | ||
| const tokenAmount = convertToTokenAmount(numberToDecimalString(amount), assetInfo.decimals); | ||
| const tokenAmount = convertToTokenAmount(amount, assetInfo.decimals); |
There was a problem hiding this comment.
High · bug — Enforce one positive-underflow policy
convertToTokenAmount truncates toward zero, so $0.0000001 currently produces amount: "0" for a 6-decimal asset here, while Exact and Upto reject the same input locally. Could we enforce a single positive-underflow policy centrally, as discussed in the earlier review, rather than leaving scheme-dependent behavior? This should also be covered by a parameterized test across all three TRON schemes.
2 participants
Description
Tests
Checklist