Remove MAX_MONEY constraint for non-Zcash chains#2
Open
AndrewKishino wants to merge 1 commit intoairgap-it:masterfrom
Open
Remove MAX_MONEY constraint for non-Zcash chains#2AndrewKishino wants to merge 1 commit intoairgap-it:masterfrom
AndrewKishino wants to merge 1 commit intoairgap-it:masterfrom
Conversation
## Problem The current implementation enforces Zcash's `MAX_MONEY` constraint (2.1 quadrillion zatoshis = 21 million ZEC) on all value balances. This limit is appropriate for Zcash but creates issues for other blockchain integrations like Tezos that use Sapling for privacy. **Specific Issue:** - Tezos tokens with 18 decimal places cannot shield amounts > 2.1 tokens - Example: A token with 18 decimals trying to shield 1.5 tokens (1.5 × 10^18 = 1,500,000,000,000,000,000 base units) exceeds the MAX_MONEY limit of 2,100,000,000,000,000 - Error message: `"Value balance is outside the range"` This is a critical limitation for DeFi applications where high-precision tokens are common. ## Solution Replace the Zcash-specific `MAX_MONEY` validation with a more permissive check that accepts the full `i64` range. Since Sapling's cryptographic operations already use `i64` internally, this change simply removes an artificial restriction without compromising security or correctness. **Trade-offs:** - ✅ Enables full `i64` range: ±9.2 quintillion (sufficient for all realistic token amounts) - ✅ Maintains type safety through `Amount` wrapper - ✅ No changes to cryptographic operations or security properties -⚠️ Removes MAX_MONEY sanity check (only relevant for Zcash mainnet) 1. The Amount type doesn't maintain MAX_MONEY invariant anyway: - From the docs: "The range is not preserved as an internal invariant" - Adding two valid Amounts can exceed MAX_MONEY - So the check is merely a constructor sanity check, not a security boundary 2. Cryptographic operations use i64 directly: - The binding_sig operation in SaplingProvingContext accepts any i64 - The Jubjub curve operations don't care about monetary policy limits 3. This is a library, not a consensus layer: - Applications using this library (like Tezos) have their own validation - The library should be flexible for different blockchain use cases 4. Precedent in zcash_primitives: - from_i64_le_bytes exists specifically for deserializing potentially-invalid amounts - The library already acknowledges that not all Amount values need MAX_MONEY validation
|
Hi @AndrewKishino, sorry for the late reply. To help us properly evaluate this change, could you provide a test scenario? Specifically:
This will help us reproduce the issue and verify that the fix works correctly before merging. Thanks! |
Author
|
Hello! After some further testing, I feel this limit is baked into the sapling code itself. I tried implementing this change locally to test and it still failed with the same issues. I can try to isolate a test scenario for you to be able to reproduce the issue. Appreciate the outreach! |
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.
Disclaimer: This change is not within my domain, but I am running into issues using this library with Tezos Sapling where tokens with many decimals (18 for example) cannot generate a sapling transaction because
Value balance is outside the range. If this is an acceptable fix for this issue, I would be happy to test this on Tezos.Problem
The current implementation enforces Zcash's
MAX_MONEYconstraint (2.1 quadrillion zatoshis = 21 million ZEC) on all value balances. This limit is appropriate for Zcash but creates issues for other blockchain integrations like Tezos that use Sapling for privacy.Specific Issue:
"Value balance is outside the range"This is a critical limitation for DeFi applications where high-precision tokens are common.
Solution
Replace the Zcash-specific
MAX_MONEYvalidation with a more permissive check that accepts the fulli64range. Since Sapling's cryptographic operations already usei64internally, this change simply removes an artificial restriction without compromising security or correctness.Trade-offs:
i64range: ±9.2 quintillion (sufficient for all realistic token amounts)Amountwrapper