-
Notifications
You must be signed in to change notification settings - Fork 181
feat: Update CryptoCreate/CryptoDelete to use new simple fees #21901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Update CryptoCreate/CryptoDelete to use new simple fees #21901
Conversation
Signed-off-by: artemderevets <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #21901 +/- ##
============================================
- Coverage 70.89% 70.82% -0.08%
+ Complexity 24429 24428 -1
============================================
Files 2668 2672 +4
Lines 104410 104531 +121
Branches 10966 10973 +7
============================================
+ Hits 74022 74031 +9
- Misses 26343 26450 +107
- Partials 4045 4050 +5
... and 14 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
…new-simple-fee' into 20443/update-crypto-services-to-new-simple-fee
Signed-off-by: artemderevets <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the PR is good and contains what I expect. The part I'm confused on is in CryptoTransferHandler. Why does it need a cache? This seems overly complex.
Also, there should be full HAPI tests for each of the transaction types.
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
I created a cache to reduce store interaction and overall complexity. This will allow to implement crypto transfrer estimation in the single pass |
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
.../java/com/hedera/node/app/service/token/impl/handlers/CryptoTransferFeeResultCalculator.java
Outdated
Show resolved
Hide resolved
.../java/com/hedera/node/app/service/token/impl/handlers/CryptoTransferFeeResultCalculator.java
Outdated
Show resolved
Hide resolved
.../java/com/hedera/node/app/service/token/impl/handlers/CryptoTransferFeeResultCalculator.java
Outdated
Show resolved
Hide resolved
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
…new-simple-fee' into 20443/update-crypto-services-to-new-simple-fee
Signed-off-by: artemderevets <[email protected]>
Signed-off-by: artemderevets <[email protected]>
hedera-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/fees/SimpleFeeCalculator.java
Show resolved
Hide resolved
hedera-node/hedera-file-service-impl/src/main/resources/genesis/simpleFeesSchedules.json
Outdated
Show resolved
Hide resolved
| final var usageMapper = usageBuilder() | ||
| .withSignatures(context.numTxnSignatures()) | ||
| .withKeys(keyCount) | ||
| .build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
| final var feeSchedule = feeCalculator.getSimpleFeesSchedule(); | ||
| final var serviceDef = lookupServiceFee(feeSchedule, HederaFunctionality.CRYPTO_CREATE); | ||
|
|
||
| // Calculate fees | ||
| long nodeFee = feeSchedule.node().baseFee() | ||
| + calculateExtraFees(feeSchedule.node().extras(), feeSchedule, usageMapper); | ||
|
|
||
| long serviceFee = serviceDef.baseFee() + calculateExtraFees(serviceDef.extras(), feeSchedule, usageMapper); | ||
|
|
||
| long networkFee = calculateNetworkFee(nodeFee, feeSchedule.network().multiplier()); | ||
|
|
||
| // Return result | ||
| final var result = new FeeResult(); | ||
| result.node = nodeFee; | ||
| result.network = networkFee; | ||
| result.service = serviceFee; | ||
| return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can all this code be moved to AbstractSimpleFeeCalculator ? Since it will be the same for every handler
...src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoCreateFeeCalculator.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoCreateFeeCalculator.java
Outdated
Show resolved
Hide resolved
| private long countKeys(Key key) { | ||
| return switch (key.key().kind()) { | ||
| case ED25519, ECDSA_SECP256K1, ECDSA_384 -> 1L; | ||
| case THRESHOLD_KEY -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We support other key types also right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we support other key types (like CONTRACT_ID, DELEGATABLE_CONTRACT_ID, RSA_3072) at the API level, but they're intentionally not counted for fee calculation. The current implementation only counts cryptographic keys (ED25519, ECDSA_SECP256K1, ECDSA_384) and recursively processes THRESHOLD_KEY and KEY_LIST. Contract ID keys return 0 because they don't contain actual key material—they're authorization references. Old fee model charges based on cryptographic complexity, not authorization complexity (as I understand). ECDSA_384 and RSA_3072 are deprecated but still handled for backward compatibility.
...src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoDeleteFeeCalculator.java
Outdated
Show resolved
Hide resolved
...-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoCreateHandler.java
Outdated
Show resolved
Hide resolved
| public boolean existsAccountWith(@NonNull final Bytes alias) { | ||
| requireNonNull(alias, "alias"); | ||
| final var hederaConfig = | ||
| feeContext.configuration().getConfigData(com.hedera.node.config.data.HederaConfig.class); | ||
| final var accountId = accountStore.getAccountIDByAlias(hederaConfig.shard(), hederaConfig.realm(), alias); | ||
| return accountId != null; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean existsAccount(@NonNull final AccountID accountId) { | ||
| requireNonNull(accountId, "accountId"); | ||
| return accountStore.getAliasedAccountById(accountId) != null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these two when we have getAccount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored. Just forget to do this
|
|
||
| @Override | ||
| public int cryptoVerificationsRequired() { | ||
| return feeContext.numTxnSignatures(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be txInfo.signatureMap().sigPair().size() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it shouldn't be txInfo.signatureMap().sigPair().size() directly. In IngestChecker.java:325: numSigs = txInfo.signatureMap().sigPair().size(). This is passed to FeeContextImpl constructor. feeContext.numTxnSignatures() returns this value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks like a great step. Thanks @aderevets . Have a few comments
Signed-off-by: artemderevets <[email protected]>
…new-simple-fee' into 20443/update-crypto-services-to-new-simple-fee
Description:
Update CryptoCreate/CryptoDelete handler to use new simple fees model based on this HIP
Update simpleFeesSchedules.json with correct values
Related issue(s):
Fixes #20443
Notes for reviewer:
Checklist