SDK audit: lift coverage to 90%, fix 7 bugs, add E2E smoke + security review#106
SDK audit: lift coverage to 90%, fix 7 bugs, add E2E smoke + security review#106gregnazario wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR is a wide-ranging audit of the SDK: it adds ~107 new unit tests (raising aptos_sdk/ coverage from 66 % → 90 %), introduces a single-command devnet smoke test, ports several correctness/safety fixes between the embedded aptos_sdk/v2 mirror and the standalone v2/src/aptos_sdk_v2 package (with a CI script to enforce parity), fixes seven concrete bugs (script-argument variant range, secp256k1 deserialization constant, indexer error handling, faucet health check, BIP-44 path validation, two examples), and adds a written security review document plus per-package Codecov flags.
Changes:
- Bug fixes across v1 (
ScriptArgumentvariants,asymmetric_crypto_wrapper.PublicKey.deserialize,IndexerClient.queryerror wrapping,FaucetClient.healthy) and v2 (BIP-44 path validation, secp256k1 wrapping inAuthenticationKey.from_public_key,ModuleId.from_strvalidation,view_bcstype-arg parsing). - New
aptos_sdk/extra_test.py(107 hermetic tests viahttpx.MockTransport), newexamples/e2e_smoke.py+make smoketarget, and v2-mirror sync CI (.github/scripts/check_v2_sync.sh). - Tooling/docs:
codecov.ymlwith splitv1-sdk/v2-sdkflags, README badge/coverage section, andSECURITY_REVIEW.mdcataloguing 7 fixed + 9 follow-up findings.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
aptos_sdk/transactions.py |
Widen ScriptArgument variant range to 0..8 and improve the error message. |
aptos_sdk/asymmetric_crypto_wrapper.py |
Fix PublicKey.deserialize to compare against PublicKey.SECP256K1_ECDSA. |
aptos_sdk/async_client.py |
Wrap indexer errors in new IndexerError; tighten FaucetClient.healthy to require HTTP 200 and swallow transport errors. |
aptos_sdk/extra_test.py |
New unit-test module covering previously-untested v1 code via mocked transport. |
v2/src/aptos_sdk_v2/crypto/mnemonic.py |
Require full 6-segment BIP-44 path and use parts[5] as address index (corrects key derivation). |
v2/src/aptos_sdk_v2/crypto/authentication_key.py |
Auto-wrap Secp256k1PublicKey into AnyPublicKey before hashing. |
v2/src/aptos_sdk_v2/transactions/payload.py |
Validate ModuleId.from_str input format. |
v2/src/aptos_sdk_v2/api/account_api.py / general_api.py |
Annotate result: Any; build view-function type args via TypeTag(StructTag.from_str(...)). |
examples/transfer_coin.py |
Treat indexer rate-limit as soft skip via IndexerError. |
examples/transaction_batching.py |
Create destination directory before Account.store writes. |
examples/fee_payer_transfer_coin.py |
Correct misleading comment about create_account. |
examples/e2e_smoke.py |
New end-to-end smoke script wired into make smoke/make examples. |
Makefile |
Add smoke target and include it in examples. |
pyproject.toml |
Exclude aptos_sdk/v2/** from v1 coverage (mirror counted in v2 suite). |
codecov.yml |
New Codecov config with v1-sdk/v2-sdk flags and per-flag targets. |
.github/workflows/lints.yaml |
Flag v1 coverage upload and run v2 mirror-sync check. |
.github/scripts/check_v2_sync.sh |
New script enforcing byte-for-byte parity between the two v2 trees. |
README.md |
Add codecov badge, smoke-test instructions, and coverage section. |
SECURITY_REVIEW.md |
New security-review document covering threat model and findings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| result = await self.client.execute_async(query, variables) | ||
| except Exception as exc: # aiohttp.ContentTypeError on rate limit, etc. | ||
| raise IndexerError(f"indexer query failed: {exc}") from exc |
| account_idx = int(parts[3]) | ||
| address_idx = int(parts[4]) | ||
| address_idx = int(parts[5]) | ||
|
|
||
| bip44_ctx = ( | ||
| Bip44.FromSeed(seed, Bip44Coins.APTOS) | ||
| .Purpose() | ||
| .Coin() | ||
| .Account(account_idx) | ||
| .Change(Bip44Changes.CHAIN_EXT) | ||
| .AddressIndex(address_idx) | ||
| ) |
| print( | ||
| "\n=== Indexer ===\n" | ||
| f"Skipped indexer assertion (no data returned). Last error: {last_error}" | ||
| ) |
Per Copilot review on PR #106: catching bare Exception was too broad and swallowed programmer bugs (TypeError, AttributeError) into IndexerError, making them harder to debug. Restrict to the actual transport / decode errors that motivated the wrapper: * aiohttp.ClientError (network + ContentTypeError on rate-limit HTML) * asyncio.TimeoutError * json.JSONDecodeError * UnicodeDecodeError Other exceptions propagate unchanged. Add a regression test that asserts TypeError is *not* converted to IndexerError. Co-authored-by: Greg Nazario <greg@gnazar.io>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
===========================================
+ Coverage 79.23% 93.14% +13.91%
===========================================
Files 56 56
Lines 5100 5138 +38
===========================================
+ Hits 4041 4786 +745
+ Misses 1059 352 -707
🚀 New features to boost your workflow:
|
| def _primitive_tag(name: str) -> TypeTag | None: | ||
| """Return the primitive ``TypeTag`` for ``name``, or ``None`` if not primitive.""" | ||
| if not _PRIMITIVE_TAGS: | ||
| # Built lazily because TypeTag isn't constructible at module import time | ||
| # for the dataclass slots dance. | ||
| _PRIMITIVE_TAGS.update( | ||
| { | ||
| "bool": TypeTag(BoolTag(False)), | ||
| "u8": TypeTag(U8Tag(0)), | ||
| "u16": TypeTag(U16Tag(0)), | ||
| "u32": TypeTag(U32Tag(0)), | ||
| "u64": TypeTag(U64Tag(0)), | ||
| "u128": TypeTag(U128Tag(0)), | ||
| "u256": TypeTag(U256Tag(0)), | ||
| "address": TypeTag(AccountAddressTag(AccountAddress(b"\x00" * 32))), | ||
| "signer": TypeTag(SignerTag()), | ||
| } | ||
| ) |
| def _primitive_tag(name: str) -> TypeTag | None: | ||
| """Return the primitive ``TypeTag`` for ``name``, or ``None`` if not primitive.""" | ||
| if not _PRIMITIVE_TAGS: | ||
| # Built lazily because TypeTag isn't constructible at module import time | ||
| # for the dataclass slots dance. | ||
| _PRIMITIVE_TAGS.update( | ||
| { | ||
| "bool": TypeTag(BoolTag(False)), | ||
| "u8": TypeTag(U8Tag(0)), | ||
| "u16": TypeTag(U16Tag(0)), | ||
| "u32": TypeTag(U32Tag(0)), | ||
| "u64": TypeTag(U64Tag(0)), | ||
| "u128": TypeTag(U128Tag(0)), | ||
| "u256": TypeTag(U256Tag(0)), | ||
| "address": TypeTag(AccountAddressTag(AccountAddress(b"\x00" * 32))), | ||
| "signer": TypeTag(SignerTag()), | ||
| } | ||
| ) |
| # Parse the BIP-44 derivation path: m/purpose'/coin'/account'/change'/address' | ||
| parts = path.replace("'", "").split("/") | ||
| if len(parts) < 5 or parts[0] != "m": | ||
| raise InvalidMnemonicError(f"Invalid derivation path: {path}") | ||
| if len(parts) < 6 or parts[0] != "m": | ||
| raise InvalidMnemonicError( | ||
| f"Invalid derivation path: {path}. " | ||
| "Expected BIP-44 format: m/44'/637'/account'/change'/address'" | ||
| ) | ||
|
|
||
| account_idx = int(parts[3]) | ||
| address_idx = int(parts[4]) | ||
| address_idx = int(parts[5]) | ||
|
|
||
| bip44_ctx = ( | ||
| Bip44.FromSeed(seed, Bip44Coins.APTOS) | ||
| .Purpose() | ||
| .Coin() | ||
| .Account(account_idx) | ||
| .Change(Bip44Changes.CHAIN_EXT) | ||
| .AddressIndex(address_idx) | ||
| ) |
| # Parse the BIP-44 derivation path: m/purpose'/coin'/account'/change'/address' | ||
| parts = path.replace("'", "").split("/") | ||
| if len(parts) < 5 or parts[0] != "m": | ||
| raise InvalidMnemonicError(f"Invalid derivation path: {path}") | ||
| if len(parts) < 6 or parts[0] != "m": | ||
| raise InvalidMnemonicError( | ||
| f"Invalid derivation path: {path}. " | ||
| "Expected BIP-44 format: m/44'/637'/account'/change'/address'" | ||
| ) | ||
|
|
||
| account_idx = int(parts[3]) | ||
| address_idx = int(parts[4]) | ||
| address_idx = int(parts[5]) | ||
|
|
||
| bip44_ctx = ( | ||
| Bip44.FromSeed(seed, Bip44Coins.APTOS) | ||
| .Purpose() | ||
| .Coin() | ||
| .Account(account_idx) | ||
| .Change(Bip44Changes.CHAIN_EXT) | ||
| .AddressIndex(address_idx) | ||
| ) |
| patcher = unittest.mock.patch( | ||
| "aptos_sdk.async_client.RestClient.account_sequence_number", return_value=0 | ||
| ) | ||
| patcher.start() | ||
| # `synchronize` reads `current_timestamp()` (which talks to the node) for | ||
| # its time-budget check; stub it so the test is fully hermetic. | ||
| timestamp_patcher = unittest.mock.patch( | ||
| "aptos_sdk.async_client.RestClient.current_timestamp", return_value=0.0 | ||
| ) | ||
| timestamp_patcher.start() | ||
|
|
||
| rest_client = RestClient("https://fullnode.devnet.aptoslabs.com/v1") | ||
| # Use a placeholder URL — the test never makes a real HTTP call. | ||
| rest_client = RestClient("http://localhost:65535") |
… range The aptos_sdk/v2 in-tree mirror had drifted ahead of the standalone v2/src/aptos_sdk_v2 package. Port forward the bug fixes (BIP-44 path validation, secp256k1 wrapping, ModuleId validation) into the standalone package so its 398 unit tests cover them, and add .github/scripts/check_v2_sync.sh to enforce the mirror invariant going forward. Also fix a latent ScriptArgument bug: the constructor rejected variants 6, 7, 8 (U16, U32, U256) even though serialize/deserialize support them. Co-authored-by: Greg Nazario <greg@gnazar.io>
Codecov was already wired up but uploaded everything under one default flag. Split coverage into v1-sdk and v2-sdk flags, configure per-flag targets, and ignore the aptos_sdk/v2 mirror so it's not double-counted (its canonical copy in v2/src/aptos_sdk_v2 is already covered by the v2-sdk flag). Also wire the new check_v2_sync.sh script into the lints workflow so a future diverging edit to either v2 tree fails CI. Add the Codecov badge to README. Co-authored-by: Greg Nazario <greg@gnazar.io>
* examples/e2e_smoke.py: a single-command health check that exercises the node,
faucet, balance reads, simulation, BCS submission, transfer_coins, account
resources, transactions_by_account, and the indexer (graceful skip on
rate-limit). Wired into 'make smoke' and 'make examples'.
* examples/transaction_batching.py: Accounts.generate('nodes', n) crashed with
FileNotFoundError because the nodes/ dir wasn't created. Add os.makedirs.
* examples/fee_payer_transfer_coin.py: comment claimed 'give Bob 1_000 coins'
but the entry function is 0x1::aptos_account::create_account. Corrected.
Co-authored-by: Greg Nazario <greg@gnazar.io>
Manual review of aptos_sdk/, v2/src/aptos_sdk_v2/, examples/, and CI workflows. Catalogs the 7 issues fixed in this PR (F-01..F-07) and 9 pre-existing findings (O-01..O-09) for follow-up, plus dependency surface and crypto notes. Co-authored-by: Greg Nazario <greg@gnazar.io>
Per Copilot review on PR #106: catching bare Exception was too broad and swallowed programmer bugs (TypeError, AttributeError) into IndexerError, making them harder to debug. Restrict to the actual transport / decode errors that motivated the wrapper: * aiohttp.ClientError (network + ContentTypeError on rate-limit HTML) * asyncio.TimeoutError * json.JSONDecodeError * UnicodeDecodeError Other exceptions propagate unchanged. Add a regression test that asserts TypeError is *not* converted to IndexerError. Co-authored-by: Greg Nazario <greg@gnazar.io>
Per Copilot review: the previous validation only checked split-arity
("::" appears exactly once), so inputs like "::foo" or "0x1::" still
produced a ModuleId with an empty address or empty name that would crash
later during BCS serialization or on the node side.
Now reject any input where either side of the "::" is empty, with the
same error message. Add tests covering all three failure shapes
("::name", "addr::", "::") plus the existing wrong-arity cases.
Mirrored into aptos_sdk/v2/.
Co-authored-by: Greg Nazario <greg@gnazar.io>
Per Copilot review: GeneralApi.view_bcs previously wrapped each entry of
ty_args in StructTag.from_str(...), so primitive type arguments such as
'u64', 'bool', 'address', or 'vector<u8>' would crash with
InvalidTypeTagError even though they are valid Move generics accepted by
the on-chain view-function ABI.
Add a top-level TypeTag.from_str parser that dispatches on the leading
token and returns:
* primitive tags (BoolTag, U8/16/32/64/128/256Tag, AccountAddressTag,
SignerTag) for 'bool', 'u8'..'u256', 'address', 'signer'
* VectorTag for 'vector<...>'
* StructTag for 'addr::module::name<...>'
Also reject:
* primitive followed by type args (e.g. 'u64<u8>')
* vector with arity != 1
* struct tags with empty address/module/name segments
* inputs that produce more than one tag at the top level
Update view_bcs to use TypeTag.from_str. Mirrored into aptos_sdk/v2/.
Adds 8 new unit tests; v2 test count is now 408.
Co-authored-by: Greg Nazario <greg@gnazar.io>
Per Copilot review: after the previous refactor the example unconditionally
swallowed indexer failures, so a regression where the indexer responded
normally but with no rows would silently pass. Restore the original
correctness guarantee:
* If we never got a successful response (last_error is not None and
we still have no data), keep the soft-skip print so devnet runs
aren't broken by indexer rate-limiting.
* Otherwise hard-assert that data is present and non-empty, so the
example continues to verify the indexer end-to-end on healthy
networks.
Co-authored-by: Greg Nazario <greg@gnazar.io>
Per Copilot review: the v2 mnemonic fix (parts[5] for address index + explicit .AddressIndex() chain call) silently changes the derived private key for any user who supplied a path with a non-zero address index. The old code read parts[4] (the *change* segment, always 0 for the canonical Aptos path) and never invoked .AddressIndex(...), so every non-zero address index collapsed onto index 0. Add a Breaking-Change entry under v2 to the Unreleased CHANGELOG section documenting the migration risk. Also document the new TypeTag.from_str, the stricter ModuleId.from_str validation, and the narrower IndexerClient.query exception scope. Co-authored-by: Greg Nazario <greg@gnazar.io>
…ermetic These two tests have always constructed a real RestClient pointed at 'https://fullnode.devnet.aptoslabs.com/v1'. They patched the obvious network methods (account_sequence_number, submit_bcs_transaction) but not the transitive ones: * AccountSequenceNumber.synchronize -> _resync -> current_timestamp -> info() makes a real GET against devnet. * TransactionWorker -> create_bcs_signed_transaction -> create_bcs_transaction -> chain_id() makes a real GET against devnet. This was a latent flaky-test bug: the suite happened to pass when devnet was healthy, and started timing out as soon as the public devnet started rate-limiting the CI / dev VMs (httpx.ReadTimeout after ~60s). Stub the missing methods (current_timestamp returns 0.0, _chain_id pre-cached to 4) and point the placeholder at http://localhost:65535 so even an accidental future regression fails fast instead of hanging on a real network roundtrip. make test now runs in ~3s with no network access. Co-authored-by: Greg Nazario <greg@gnazar.io>
9be0db1 to
d487566
Compare
Three files were out of sync between aptos_sdk/v2 (mirror) and v2/src/aptos_sdk_v2 (canonical), causing check_v2_sync.sh to exit 1: * bcs/serializer.py: mirror had old map-serialization logic that encoded only keys before sorting; canonical correctly encodes both key and value before sorting. Port canonical fix into the mirror. * transactions/raw_transaction.py: mirror used module-level constants for the prehash bytes; canonical refactored to lazy functions with docstrings. Port canonical form into the mirror. * types/type_tag.py: the lru_cache(maxsize=256) optimization on StructTag.from_str was present in the mirror but absent from the canonical. Add it to the canonical, then rsync canonical → mirror so all three files are byte-identical.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
v2/src/aptos_sdk_v2/types/type_tag.py:229
StructTag.from_stris now@lru_cache’d but returns a mutableStructTag(not frozen; contains a mutabletype_argslist). Any accidental mutation (e.g.tag.type_args.append(...)) would corrupt the cached instance for subsequent calls. Either makeStructTagimmutable (e.g.frozen=Trueand storetype_argsas a tuple) or have the cached function return a deep-copied/new instance.
@staticmethod
@lru_cache(maxsize=256)
def from_str(type_tag: str) -> StructTag:
tags, _ = _parse_type_tags(type_tag, 0)
if not tags: # pragma: no cover — parser always appends via _make_struct_tag
raise InvalidTypeTagError(f"Cannot parse type tag: {type_tag}")
inner = tags[0].value
if not isinstance(inner, StructTag): # pragma: no cover — parser only creates StructTags
raise InvalidTypeTagError(f"Expected StructTag, got {type(inner).__name__}")
return inner
| # Parse the BIP-44 derivation path: m/purpose'/coin'/account'/change'/address' | ||
| parts = path.replace("'", "").split("/") | ||
| if len(parts) < 5 or parts[0] != "m": | ||
| raise InvalidMnemonicError(f"Invalid derivation path: {path}") | ||
| if len(parts) < 6 or parts[0] != "m": | ||
| raise InvalidMnemonicError( | ||
| f"Invalid derivation path: {path}. " | ||
| "Expected BIP-44 format: m/44'/637'/account'/change'/address'" | ||
| ) |
| # Parse the BIP-44 derivation path: m/purpose'/coin'/account'/change'/address' | ||
| parts = path.replace("'", "").split("/") | ||
| if len(parts) < 5 or parts[0] != "m": | ||
| raise InvalidMnemonicError(f"Invalid derivation path: {path}") | ||
| if len(parts) < 6 or parts[0] != "m": | ||
| raise InvalidMnemonicError( | ||
| f"Invalid derivation path: {path}. " | ||
| "Expected BIP-44 format: m/44'/637'/account'/change'/address'" | ||
| ) |
| _PRIMITIVE_TAGS: dict[str, TypeTag] = {} | ||
|
|
||
|
|
||
| def _primitive_tag(name: str) -> TypeTag | None: | ||
| """Return the primitive ``TypeTag`` for ``name``, or ``None`` if not primitive.""" | ||
| if not _PRIMITIVE_TAGS: | ||
| # Built lazily because TypeTag isn't constructible at module import time | ||
| # for the dataclass slots dance. | ||
| _PRIMITIVE_TAGS.update( | ||
| { | ||
| "bool": TypeTag(BoolTag(False)), | ||
| "u8": TypeTag(U8Tag(0)), | ||
| "u16": TypeTag(U16Tag(0)), | ||
| "u32": TypeTag(U32Tag(0)), | ||
| "u64": TypeTag(U64Tag(0)), | ||
| "u128": TypeTag(U128Tag(0)), | ||
| "u256": TypeTag(U256Tag(0)), | ||
| "address": TypeTag(AccountAddressTag(AccountAddress(b"\x00" * 32))), | ||
| "signer": TypeTag(SignerTag()), | ||
| } | ||
| ) | ||
| return _PRIMITIVE_TAGS.get(name) |
| def _raw_txn_prehash() -> bytes: | ||
| """SHA3-256 of b"APTOS::RawTransaction" — the domain separator for single-sender signing.""" | ||
| hasher = hashlib.sha3_256() | ||
| hasher.update(b"APTOS::RawTransaction") | ||
| return hasher.digest() | ||
|
|
||
|
|
||
| def _raw_txn_with_data_prehash() -> bytes: | ||
| """SHA3-256 of b"APTOS::RawTransactionWithData" — for multi-agent/fee-payer signing.""" | ||
| hasher = hashlib.sha3_256() | ||
| hasher.update(b"APTOS::RawTransactionWithData") | ||
| return hasher.digest() |
| # Files under aptos_sdk/v2/ are a mirror of v2/src/aptos_sdk_v2; don't | ||
| # double-count them — the v2-sdk flag covers the canonical copy. | ||
| # Files under aptos_sdk/v2/ are byte-for-byte mirrors of v2/src/aptos_sdk_v2; | ||
| # don't double-count them — the v2 flag covers the canonical copy. |
Description
Full audit of the SDK requested in the task. Highlights:
aptos_sdk/raised from 66 % → 90 % (make test-coverage). Test count went from 54 to 161. All v1 modules except CLI subprocess wrappers now sit at ≥ 80 %; previously-untouched modules (async_client,aptos_token_client,aptos_tokenv1_client,package_publisher,cli) all gained dedicated unit tests usinghttpx.MockTransportinstead of network calls so they run in < 3 s.Testclasses already had real assertions; the newaptos_sdk/extra_test.py(107 tests) follows the same pattern. The file uses the*_test.pynaming so it is excluded from coverage counting (matches theomitglob inpyproject.toml).aptos_token,fee_payer_transfer_coin,multikey,rotate_key,read_aggregator,secp256k1_ecdsa_transfer_coin,simple_aptos_token,simple_nft,simulate_transfer_coin,transfer_coin,transfer_two_by_two. Two example bugs fixed (see F-06/F-07 below).examples/e2e_smoke.pywired intomake smokeandmake examples— a single-command health check exercising 8 layers of the stack (info,chain_id,faucet,balance,simulate,bcs_transfer,transfer_coins,account_resources/transactions_by_account, indexer with graceful rate-limit skip).SECURITY_REVIEW.md: 7 issues fixed in this PR (F-01..F-07) and 9 pre-existing findings catalogued for follow-up (O-01..O-09).codecov.ymlwith splitv1-sdk/v2-sdkflags, ignores theaptos_sdk/v2mirror so it isn't double-counted, sets per-flag targets (88 % / 95 %), and adds the badge to the README.Bugs fixed in this PR
ScriptArgument.__init__rejected variants 6/7/8 (U16/U32/U256) even thoughserialize/deserializesupported them.asymmetric_crypto_wrapper.PublicKey.deserializereferencedSignature.SECP256K1_ECDSAinstead ofPublicKey.SECP256K1_ECDSA(worked by accident — both equal1).IndexerClient.querypropagated rawaiohttp.ContentTypeErrorwhen the indexer returned an HTML rate-limit page; now wrapped in a newIndexerError, which also catches GraphQL-levelerrors.FaucetClient.healthyaccepted any 5xx response containingtap:okand crashed on transport errors. Now requiresstatus_code == 200.aptos_sdk/v2in-tree mirror had drifted ahead of the standalonev2/src/aptos_sdk_v2package (BIP-44 path validation, secp256k1 wrapping,ModuleId.from_strvalidation). Ported the fixes back, then added.github/scripts/check_v2_sync.shand a CI step to enforce mirror parity going forward.examples/transaction_batching.pycrashed withFileNotFoundErrorbecausenodes/wasn't created.examples/fee_payer_transfer_coin.pycomment claimed "give Bob 1_000 coins" but the entry function is0x1::aptos_account::create_account.Test Plan
Coverage delta
aptos_sdk/overallaptos_token_client.pyaptos_tokenv1_client.pypackage_publisher.pycli.pyasync_client.pytransactions.pytype_tag.pyaccount.pyRelated Links
SECURITY_REVIEW.md(added in this PR)examples/e2e_smoke.py(added in this PR).github/scripts/check_v2_sync.sh(added in this PR)