Raise instead of warn when multi-sig sign types cannot be enriched - #315
Open
BhariGowda wants to merge 1 commit into
Open
Raise instead of warn when multi-sig sign types cannot be enriched#315BhariGowda wants to merge 1 commit into
BhariGowda wants to merge 1 commit into
Conversation
add_multi_sig_types inserts payloadMultiSigUser and outerSigner after the
hyperliquidChain entry. When sign_types has no hyperliquidChain entry it
printed a warning and returned the list unchanged, then signing carried
on with the un-enriched types.
eth_account builds the EIP-712 hash from the type list and ignores
message keys with no matching type entry, so add_multi_sig_fields still
puts payloadMultiSigUser and outerSigner into the message and they are
silently dropped from the hash. The result is a signature identical to a
plain single-signer one for the same action:
BAD = [t for t in USD_SEND_SIGN_TYPES if t["name"] != "hyperliquidChain"]
sign_multi_sig_user_signed_action_payload(w, a, False, BAD, ..., msu, outer)
== sign_user_signed_action(w, a, BAD, ..., False) # True
It carries no binding to the multi-sig user or the outer signer, so it is
not usable as an inner multi-sig signature. All of the sign types
exported from this module have hyperliquidChain first, so this only
affects callers passing their own type list, which is the case for any
action the SDK does not have a helper for yet. A stdout warning is easy
to miss there.
Raising ValueError instead. No signature that the chain accepts today
changes, since the un-enriched path could not produce one.
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
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.
add_multi_sig_typesinsertspayloadMultiSigUserandouterSignerafter thehyperliquidChainentry. Whensign_typeshas nohyperliquidChainentry it printed a warning and returned the list unchanged, and signing then carried on with the un-enriched types.That does not fail loudly.
eth_accountbuilds the EIP-712 hash from the type list and ignores message keys that have no matching type entry, soadd_multi_sig_fieldsstill putspayloadMultiSigUserandouterSignerinto the message and they are silently dropped from the hash. The signature that comes out is byte-identical to a plain single-signer one for the same action:Both produce
r=0x31d2170f…,s=0x1f3c5743…,v=28.Since it carries no binding to the multi-sig user or the outer signer, it is not usable as an inner multi-sig signature. Every sign-types list exported from this module has
hyperliquidChainfirst, so this only affects callers passing their own type list, which is the case for any action the SDK does not have a helper for yet. A warning on stdout is easy to miss there, and the failure surfaces later as a rejected action with nothing pointing back at the cause.Two things I checked before changing this to raise.
It is not a quirk of one
eth_accountrelease. The two signatures come out byte-identical, samerand sames, on 0.10.0, 0.11.3, 0.12.3 and 0.13.7, which covers the whole range this package allows (eth-account = ">=0.10.0,<0.14.0").There is no valid user-signed action whose types legitimately omit
hyperliquidChain, so raising here cannot break a configuration that works today. All 10 sign-types constants exported fromhyperliquid/utils/signing.pyhave it as their first field. I also went through the third-party TypeScript SDKnktkas/hyperliquid, which models 17 user-signed actions including 6 this SDK has no helper for yet (cDeposit,cWithdraw,linkStakingUser,sendToEvmWithData,stakingLinkDisableTradingUser,userPortfolioMargin), and all 17 havehyperliquidChainfirst as well. Reaching the un-enriched branch is therefore always a caller mistake rather than a legitimate case the SDK needs to keep supporting.Raising
ValueErrorinstead. No signature that the chain accepts today changes, because the un-enriched path could not produce one.The test asserts that both
add_multi_sig_typesandsign_multi_sig_user_signed_action_payloadraise on a type list with nohyperliquidChain, and pins the enriched field order for the normal case.Ran
pytest tests/signing_test.py(14 passed) plus black, isort and flake8 with the repo's configured settings.