feat(precompiles): TIP-1026 add logoURI and createToken overload T5+#3745
feat(precompiles): TIP-1026 add logoURI and createToken overload T5+#3745
Conversation
|
📊 Tempo Precompiles CoverageprecompilesCoverage: 5722/7784 lines (73.51%) File details
contractsCoverage: 1/269 lines (0.37%) File details
Total: 5723/8053 lines (71.07%) |
|
/build-devnet |
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
TIP-1026 implementation looks solid: T5 selector gating, role check, validate-then-deploy ordering in the factory, storage append-only, and legacy 6-arg createToken selector / TokenCreated topic preservation are all enforced and covered by unit + Solidity invariant tests. No exploitable security issues across three independent worker passes. The findings below are integration / hygiene risks, surfaced as inline comments where they map to the diff plus two issues against files outside the diff.
⚠️ [ISSUE] Solidity ↔ Rust storage layout test silently bypasses the new field (file not in diff)
Files: crates/precompiles/tests/storage_tests/solidity/testdata/tip20.sol and crates/precompiles/tests/storage_tests/solidity/precompiles.rs:143
The Rust TIP20Token struct now has logo_uri: String appended, but neither the Solidity reference fixture (testdata/tip20.sol) nor the layout_fields! enumeration in test_tip20_layout was updated. compare_layouts only iterates the field list it is given, so the layout-equivalence test passes without checking that logoURI lands on the same slot on both sides. This silently weakens the protection ZELLIC-23 was meant to give, and any future contributor inserting Solidity logoURI somewhere other than the end will desync the layout without a CI signal.
Recommended Fix: Add string logoURI; as the last field of tests/storage_tests/solidity/testdata/tip20.sol, and add logo_uri to the layout_fields! invocation in tests/storage_tests/solidity/precompiles.rs (both the test_tip20_layout site near line 163 and export_all_storage_constants near line 363).
⚠️ [ISSUE] tempo-contracts ships a SemVer-breaking Rust API rename without a major bump (file not in diff)
File: crates/contracts/Cargo.toml (publish = true, version 1.6.0)
alloy::sol! now sees two createToken overloads and renames the generated bindings to createToken_0Call / createToken_1Call (and factory.createToken_0(…) / factory.createToken_1(…) on the contract handle). The PR re-exports createToken_0Call as createTokenCall at tempo_contracts::precompiles, but consumers that imported via the inner path (tempo_contracts::precompiles::ITIP20Factory::createTokenCall) or used the alloy contract instance method factory.createToken(…) will fail to compile. This is a breaking change for downstream Rust consumers of the published crate; the version remains 1.6.0.
Recommended Fix: Bump tempo-contracts to 2.0.0 for the next release and note the rename in the changelog. Optionally add pub use createToken_0Call as createTokenCall inside ITIP20Factory to keep the inner-path import working.
Reviewer Callouts
- ⚡
data:scheme allowed. The validator only inspects the scheme prefix; everything past the first:is unchecked (NUL bytes, control chars, bidi marks, percent-encoded payloads,data:text/html,<script>…). This is the documented design — wallet/UI/explorer teams are responsible for sandboxing rendering. Worth one explicit confirmation that frontend treatsdata:URIs read from token storage as untrusted. - ⚡ String storage shrink leaves orphan data. Overwriting a long URI with a shorter/empty one writes only the base slot; the keccak-derived data slots retain previous bytes. Reads remain correct (length is from the base slot), but any future “compact storage” or SSTORE-refund work that recomputes length from data slots would resurface stale bytes.
- ⚡ Gas pricing for
setLogoURI/createTokenWithLogo. Charges onlyINPUT_PER_WORD_COST × ceil(calldata/32)plus SSTORE costs. Sanity-check that this matches comparable TIP-20 admin ops (setSupplyCap,changeTransferPolicyId). - ⚡ Factory
write_logo_uriskips the role check. Intentional (creator authoring initial state for a token they just deployed) and matches the threat model of the legacy 6-argcreateToken. No new attack surface, but worth keeping in mind when reasoning aboutLogoURIUpdated.updatersemantics.
| opted_in_supply: u128, | ||
| user_reward_info: Mapping<Address, UserRewardInfo>, | ||
|
|
||
| // TIP-1026: Token Logo URI (must remain at the end — storage is append-only) |
There was a problem hiding this comment.
this is indeed valid, but a more generic problem not specific to this PR. in any case reordering of slots would be caught by snapshot reexecution, we can address compile-time checks separately. added logo_uri to the layout equivalence test here 42da069
There was a problem hiding this comment.
assuming that they have always been unused, which i think it is the case, you can technically of the unused slots (i.e. _domain_separator)
56211b4 to
42da069
Compare
| opted_in_supply: u128, | ||
| user_reward_info: Mapping<Address, UserRewardInfo>, | ||
|
|
||
| // TIP-1026: Token Logo URI (must remain at the end — storage is append-only) |
There was a problem hiding this comment.
this is indeed valid, but a more generic problem not specific to this PR. in any case reordering of slots would be caught by snapshot reexecution, we can address compile-time checks separately. added logo_uri to the layout equivalence test here 42da069
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review — No actionable findings.
Three workers ran six independent passes (invariant tracing, blast-radius mapping, domain coverage A–D, and an invariant-breaker pass) over TIP-1026 (Token Logo URI) and all returned no findings. Validation (256-byte cap + scheme allowlist), T5 selector gating, append-only slot 18 layout, atomic factory creation, admin-gated setLogoURI, and the pinned legacy createToken selector (0x68130445) / TokenCreated topic0 all check out.
Reviewer Callouts
- ⚡ T5 activation timing:
TempoHardfork::T5still hasNoneactivation incrates/chainspec/src/hardfork.rs. Confirm the activation timestamp ships in lockstep with these selectors and that moderato exercisescreateTokenWithLogobefore mainnet activation. - ⚡ Storage-layout append-only contract: any future TIP-20 storage field MUST be appended after
logo_uri(slot 18). The only safety net is the Solidity layout-equivalence test (crates/precompiles/tests/storage_tests/solidity/precompiles.rs). - ⚡
createTokenoverload-ordering fragility: alloy disambiguates by declaration order (createToken_0= legacy 6-arg,createToken_1= 7-arg). Reordering the two declarations incrates/contracts/src/precompiles/tip20_factory.rswould silently swap dispatch routing. The selector-pin testtest_create_token_selector_and_event_unchangedand the FoundryLEGACY_CREATE_TOKEN_SELECTORinvariant catch this — keep both. - ⚡ Lossy UTF-8 decoding: alloy's
SolType for Stringusesfrom_utf8_lossy, so malformed UTF-8 calldata is silently lossy-decoded (and may expand up to 3×). The length cap is enforced after decode so the on-chain invariant holds, but any future change using byte-exactstringsemantics (signed payloads, hash-keyed storage) should opt intoabi_decode_validate. - ⚡ Empty-URI semantics asymmetry:
setLogoURI("")writes the slot and emitsLogoURIUpdated(_, "");createToken(..., "")skips both. Intentional and tested, but confirm downstream indexers don't rely on a deploy-timeLogoURIUpdatedfor every token. - ⚡
Tip20TokenMetadata(RPCsimulate.rs) does not yet exposelogoURIpost-T5; dashboards relying on RPC for token metadata must query the precompile directly until that is added. - ⚡ Stale long-string chunks on shrink: shrinking a previously-long
logo_urileaves old chunk slots atkeccak256(18)+inon-zero (existingBytesLikeHandlerpattern, also affectsname/symbol/currency). Not exploitable sincelogoURI()reads onlylengthchunks.
… write_logo_uri auth contract
7ea2fb8 to
c627a36
Compare
… empty-URI factory/setter asymmetry
…undant integration tests
| opted_in_supply: u128, | ||
| user_reward_info: Mapping<Address, UserRewardInfo>, | ||
|
|
||
| // TIP-1026: Token Logo URI (must remain at the end — storage is append-only) |
There was a problem hiding this comment.
assuming that they have always been unused, which i think it is the case, you can technically of the unused slots (i.e. _domain_separator)
Co-authored-by: 0xrusowsky <90208954+0xrusowsky@users.noreply.github.com>
…instead of appending
good point, done here 553fb5c |
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review — No actionable findings.
TIP-1026 adds logoURI() / setLogoURI(string) (T5+, admin-gated) and a 7-arg createToken(...,string logoURI) factory overload, repurposing the previously-unused _domain_separator slot 5 as a Solidity string. Validation enforces a 256-byte cap and an ASCII-case-insensitive scheme allowlist (https/http/ipfs/data); empty strings clear. Legacy createToken selector 0x68130445 and TokenCreated topic are preserved via Solidity overload resolution (createToken_0/createToken_1). Two workers across four iterations independently reached NO_FINDING. The slot-5 reuse — the highest-risk change — is safe: the legacy writer was removed in PR #580 (2025-10-29) months before any live genesis (Moderato 2026-02-05, Presto 2026-02-12), domain_separator() is computed dynamically, initialize() never touches slot 5, genesis state leaves it unset, and a zero slot decodes deterministically to "" per Solidity short-string encoding.
Reviewer Callouts
- ⚡ Storage-slot reuse (slot 5): The "always 0 on pre-T5 tokens" invariant depends on no consensus path having ever written
_domain_separatoron a network whose state is carried forward. Verified for live chains, but a human should confirm there are no internal/dev/private snapshots being migrated forward where this might not hold. If any exist, a one-time T5-boundary normalization that explicitly zeroes slot 5 for every TIP-20 would be the safe play. An off-chain post-T5 sanity script is a cheap defense-in-depth. - ⚡
write_logo_uriispub(crate)and intentionally unguarded: The doc-comment is explicit ("caller's responsibility"), but a future maintainer adding a new caller could miss it. Consider renaming towrite_logo_uri_uncheckedor tightening visibility further (e.g., crate-private totip20_factoryonly). - ⚡ Pre-T5 layout coverage of genesis-installed tokens:
test_logo_uri_pre_t5_deploy_post_t5_read_returns_emptyexercises factory-deployed tokens. A genesis-installed token (e.g.,pathUSD) would be worth covering explicitly to pin the assumption that genesis slot 5 stays zero. - ⚡ Solidity layout-equivalence test toolchain: The
tip20.layout.jsontest requiressolc. Confirm CI runs the layout test with the right toolchain so any future struct reordering catches a mismatch immediately. - ⚡ TIP-1026 factory error precedence:
create_token_with_logovalidates the URI beforecreate_token, soLogoURITooLong/InvalidLogoURImasks would-beTokenAlreadyExists/InvalidQuoteTokenreverts. Reasonable, but worth documenting if integrators rely on a specific revert reason. - ⚡ Long-string state residue: When
set_logo_urireplaces a long URI (≥32 bytes) with a shorter (or empty) value, residual chunks atkeccak256(slot_5)+ipast the new length are not freed. Symmetric to existingname/symbolbehavior; not exploitable, but a future cleanup pass could calldelete_bytes_likefirst. - ⚡ Empty-string semantics asymmetry:
setLogoURI("")validates and emitsLogoURIUpdated;createTokenWithLogoskips both write and event whenlogoURIis empty. Intentional per spec, but indexers tracking logo updates must understand the deploy event is conditional on non-empty input. - ⚡ Mainnet T5 activation: T5 has no mainnet activation block yet (
crates/chainspec/src/hardfork.rs); behavior is dormant in production. Confirm nothing else in this PR depends on a specific mainnet T5 height.
|
/build-devnet |
ref #2996
ref https://linear.app/tempoxyz/issue/CHAIN-1142/tip-1026-optional-logouri-field-in-tip-20-token
Adds a
logoURIfield to TIP-20 tokens with asetLogoURIadmin function and a 7-argTIP20Factory.createTokenSolidity overload, both gated behind T5. The URI is capped at 256 bytes and validated against a scheme allowlist(
https,http,ipfs,data, ASCII-case-insensitive); rejection surfaces asLogoURITooLongorInvalidLogoURI. Empty strings are valid:setLogoURI("")clears the URI and emitsLogoURIUpdated; the factory overload skips both the slot write and the event whenlogoURIis empty (per spec).The legacy 6-arg
createTokenselector (0x68130445) and theTokenCreatedevent topic0 are unchanged — pinned by a regression test. The factory does not introduce a factory-level event; at deploy time it emits the token'sLogoURIUpdatedfrom the new token's address withupdater = msg.sender.