feat: implement hss sign schedule function #16789

lukelee-sl · 2024-11-26T03:51:06Z

Description:
Implements the signSchedule proxy contract function and the authorizeSchedule function for the Schedule system contract.

Related issue(s):

Fixes #16700

Notes for reviewer:
A future pr will add additional BDD and unit test.

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Signed-off-by: Luke Lee <[email protected]>

codecov · 2024-11-26T04:02:06Z

Codecov Report

Attention: Patch coverage is 72.34043% with 26 lines in your changes missing coverage. Please review.

Project coverage is 63.63%. Comparing base (32089ae) to head (3d87889).

Files with missing lines	Patch %	Lines
...racts/hss/signschedule/SignScheduleTranslator.java	64.40%	15 Missing and 6 partials ⚠️
...mcontracts/hss/DispatchForResponseCodeHssCall.java	92.00%	1 Missing and 1 partial ⚠️
...rvice/contract/impl/exec/TransactionProcessor.java	66.66%	0 Missing and 1 partial ⚠️
...mpl/exec/scope/HandleSystemContractOperations.java	0.00%	1 Missing ⚠️
...ec/systemcontracts/common/AbstractCallAttempt.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             develop   #16789   +/-   ##
==========================================
  Coverage      63.62%   63.63%           
- Complexity     20401    20418   +17     
==========================================
  Files           2537     2537           
  Lines          94757    94842   +85     
  Branches        9904     9916   +12     
==========================================
+ Hits           60288    60348   +60     
- Misses         30863    30878   +15     
- Partials        3606     3616   +10

Files with missing lines	Coverage Δ
...orkflows/handle/dispatch/ChildDispatchFactory.java	`91.30% <100.00%> (+0.86%)`	⬆️
...a/com/hedera/node/config/data/ContractsConfig.java	`100.00% <ø> (ø)`
...p/service/contract/impl/exec/gas/DispatchType.java	`100.00% <100.00%> (ø)`
...impl/exec/scope/QuerySystemContractOperations.java	`100.00% <100.00%> (ø)`
...ract/impl/exec/scope/SystemContractOperations.java	`100.00% <ø> (ø)`
...t/impl/exec/systemcontracts/HssSystemContract.java	`66.66% <ø> (ø)`
.../contract/impl/state/DispatchingEvmFrameState.java	`89.47% <ø> (ø)`
...rvice/contract/impl/exec/TransactionProcessor.java	`94.38% <66.66%> (+0.12%)`	⬆️
...mpl/exec/scope/HandleSystemContractOperations.java	`95.12% <0.00%> (-2.38%)`	⬇️
...ec/systemcontracts/common/AbstractCallAttempt.java	`94.73% <0.00%> (-1.70%)`	⬇️
... and 2 more

... and 3 files with indirect coverage changes

codacy-production · 2024-11-26T04:02:16Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.01% (target: -1.00%)	✅ 80.85%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`32089ae`)	97522	63704	65.32%
Head commit (`3d87889`)	97607 (+85)	63774 (+70)	65.34% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#16789)	94	76	80.85%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

stoyanov-st

Overall LGTM, ty @lukelee-sl !
Left a few suggestions for improvement but all up to you

...-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/TransactionProcessor.java

hedera-node/hedera-config/src/main/java/com/hedera/node/config/data/ContractsConfig.java

.../app/service/contract/impl/exec/systemcontracts/hss/signschedule/SignScheduleTranslator.java

Signed-off-by: Luke Lee <[email protected]>

stoyanov-st

LGTM

…into 16700-implement-sign-schedule

david-bakin-sl

Will come back for tests (bdd) later. Some questions for now.

david-bakin-sl · 2024-11-27T17:23:25Z

...pp/src/test/java/com/hedera/node/app/workflows/handle/dispatch/ChildDispatchFactoryTest.java

+        assertThat(derivedVerifier.authorizingSimpleKeys()).containsExactly(A_CONTRACT_ID_KEY);
+        assertThat(derivedVerifier.authorizingSimpleKeys()).isNotEmpty();
+    }
+
    @Test
    void keyVerifierOnlySupportsKeyVerification() {


Given this test what does this test's name mean? (I know you didn't name it, but you could fix the name anyway, if I'm not just confused.)

...-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/TransactionProcessor.java

david-bakin-sl · 2024-11-27T17:44:47Z

...-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/TransactionProcessor.java

@@ -258,8 +258,9 @@ private InvolvedParties computeInvolvedParties(
    }

    private boolean contractNotRequired(@Nullable final HederaEvmAccount to, @NonNull final Configuration config) {
-        final var maybeGrandfatheredNumber =
-                (to == null) ? null : to.isTokenFacade() ? null : to.hederaId().accountNumOrThrow();
+        final var maybeGrandfatheredNumber = (to == null || to.isTokenFacade() || to.isScheduleTxnFacade())


Also I was curious why this change didn't have a corresponding test to modify in TransactionProcessorTest but I see it's basically not unit-testable: It's a private method nested 3 levels deep in calls from other private methods. You basically can only test this little method by starting at the top-level method in this class (processTransaction) and maneuvering to drive private method computeInvolvedParties to the exact required state and inspecting the resulting HederaEvmTransactionResult. After mocking all dependencies of actually doing the transaction you're submitting and also for computing gas.

private methods make classes untestable, most often, in my experience. And it doesn't add much in the way of "safety" in terms of not using things that weren't meant to be public. If the original dev didn't want the stuff to be public he should have isolated it behind an interface for the public methods, leaving the formerly-private methods available publicly in the ...Impl so they could be tested.

david-bakin-sl · 2024-11-27T17:55:04Z

.../java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hss/HssCallAttempt.java

@@ -63,7 +68,8 @@ public HssCallAttempt(
            @NonNull final VerificationStrategies verificationStrategies,
            @NonNull final SystemContractGasCalculator gasCalculator,
            @NonNull final List<CallTranslator<HssCallAttempt>> callTranslators,
-            final boolean isStaticCall) {
+            final boolean isStaticCall,
+            @NonNull final Address originatorAddress) {


Should this be moved up, perhaps under senderAddress?

Michael, convinced me that originatorAddress is not really needed. I will remove it.

david-bakin-sl · 2024-11-27T18:07:00Z

.../node/app/service/contract/impl/exec/systemcontracts/hss/DispatchForResponseCodeHssCall.java

+            return gasOnly(
+                    haltResult(
+                            ERROR_DECODING_PRECOMPILE_INPUT,
+                            contractsConfigOf(frame).precompileHtsDefaultGasCost()),


Should there be a config for precompileHssDefaultGasGost? (Hss)

I don't think it is worth it to have a different gas cost for this for each system contract.

Maybe should be renamed?

david-bakin-sl · 2024-11-27T18:08:26Z

.../node/app/service/contract/impl/exec/systemcontracts/hss/DispatchForResponseCodeHssCall.java

+ * An HSS call that simply dispatches a synthetic transaction body and returns a result that is
+ * an encoded {@link ResponseCodeEnum}.
+ */
+public class DispatchForResponseCodeHssCall extends AbstractCall {


So close to DispatchForResponseCodeHtsCall and yet not close enough to derive from same abstract class. (Or is it close enough?)

I didn't want all of the clutter if possible. Maybe I'll create an abstract class if it turns out the addition updaters are needed here.

.../app/service/contract/impl/exec/systemcontracts/hss/signschedule/SignScheduleTranslator.java

david-bakin-sl · 2024-11-27T18:24:12Z

.../app/service/contract/impl/exec/systemcontracts/hss/signschedule/SignScheduleTranslator.java

+        }
+        key = attempt.enhancement().nativeOperations().getAccountKey(attempt.originatorAccount());
+        if (key != null) {
+            return Set.of(key);


What if this account key is not a simple key. Isn't this eventually what is sent to ChildDispatchFactory where it becomes what's returned by a key verifiers authorizingSimpleKeys? Wouldn't that mean it needs to be a simple key?

I image that the key will be passed into the verifier and as long as the verifier can handle key lists for verification then it should work? It looks like the DefaultKeyVerifier which is what I am passing does allow verification of things like key lists but not sure here. @tinker-michaelj ?

david-bakin-sl · 2024-11-27T18:30:11Z

.../src/main/java/com/hedera/node/app/service/contract/impl/state/DispatchingEvmFrameState.java

@@ -110,7 +110,7 @@ public class DispatchingEvmFrameState implements EvmFrameState {
    // The only exception is that the function selector for `redirectForScheduleTxn` (0x5c3889ca)
    // has been pre substituted before the ADDRESS_BYTECODE_PATTERN.
    private static final String SCHEDULE_CALL_REDIRECT_CONTRACT_BINARY =
-            "6080604052348015600f57600080fd5b50600061016a9050775c3889cafefefefefefefefefefefefefefefefefefefefe600052366000602037600080366018016008845af43d806000803e8160008114605857816000f35b816000fdfea2646970667358221220d8378feed472ba49a0005514ef7087017f707b45fb9bf56bb81bb93ff19a238b64736f6c634300080b0033";
+            "6080604052348015600f57600080fd5b50600061016b9050775c3889cafefefefefefefefefefefefefefefefefefefefe600052366000602037600080366018016008845af43d806000803e8160008114605857816000f35b816000fdfea2646970667358221220d8378feed472ba49a0005514ef7087017f707b45fb9bf56bb81bb93ff19a238b64736f6c634300080b0033";


I detect one letter in that hex string changed. Don't even know if it is the right change. I suggest again: This is the 4 byte hex selector that needs to be substituted into these three otherwise identical 147 byte hex strings. Can't we have code that takes the one true proxy binary and injects that selector into it producing these three static strings?

well there's actually 2 changes. I see what I can do.

Signed-off-by: Luke Lee <[email protected]>

david-bakin-sl

LGTM (though there's still a question about simple keys remaining for completeness)

david-bakin-sl · 2024-11-27T22:30:18Z

...pp/src/test/java/com/hedera/node/app/workflows/handle/dispatch/ChildDispatchFactoryTest.java

+        assertThat(derivedVerifier.authorizingSimpleKeys()).containsExactly(A_CONTRACT_ID_KEY);
+        assertThat(derivedVerifier.authorizingSimpleKeys()).isNotEmpty();


FYI, assertThat has a "fluid" API, you can write

assertThat(derivedVerifier.authorizingSimpleKeys()).isNotEmpty().containsExactly(A_CONTRACT_ID_KEY);

(though in fact that's redundant in this particular case, don't need the isNotEmpty() here)
(in general should be containsOnly as even if this test had more then one element to be contained the order doesn't matter)
(just spitting out AssertJ stuff for posterity, don't even think about it.)

...-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/TransactionProcessor.java

david-bakin-sl · 2024-11-27T22:49:20Z

.../src/main/java/com/hedera/node/app/service/contract/impl/state/DispatchingEvmFrameState.java

@@ -91,26 +91,40 @@ public class DispatchingEvmFrameState implements EvmFrameState {

    private static final String ADDRESS_BYTECODE_PATTERN = "fefefefefefefefefefefefefefefefefefefefe";

+    private static final String PROXY_PRE_BYTES = "6080604052348015600f57600080fd5b506000610";


I do like this version better. I was actually thinking of something like this (two alternatives here, 1 and 2):

private static final String PROXY_PRE_BYTES = "6080604052348015600f57600080fd5b506000610"; private static final String PROXY_MID_BYTES = "905077"; private static final String PROXY_POST_BYTES = "fefefefefefefefefefefefefefefefefefefefe600052366000602037600080366018016008845af43d806000803e8160008114605857816000f35b816000fdfea2646970667358221220d8378feed472ba49a0005514ef7087017f707b45fb9bf56bb81bb93ff19a238b64736f6c634300080b0033"; private static final BiFunction<String, String, String> constructProxy1 = (@NonNull final String systemContractAddress, @NonNull final String functionSelectorHex) -> PROXY_PRE_BYTES + systemContractAddress + PROXY_MID_BYTES + functionSelectorHex + PROXY_POST_BYTES; private static final String PROXY_METHOD_TEMPLATE = "6080604052348015600f57600080fd5b506000610%1$s905077%2$sfefefefefefefefefefefefefefefefefefefefe600052366000602037600080366018016008845af43d806000803e8160008114605857816000f35b816000fdfea2646970667358221220d8378feed472ba49a0005514ef7087017f707b45fb9bf56bb81bb93ff19a238b64736f6c634300080b0033"; private static final BiFunction<String, String, String> constructProxy2 = (@NonNull final String systemContractAddress, @NonNull final String functionSelectorHex) -> PROXY_METHOD_TEMPLATE.formatted(systemContractAddress, functionSelectorHex); private static final String TOKEN_CALL_REDIRECT_CONTRACT_BINARY = constructProxy1.apply("0167", "618dc65e"); private static final String ACCOUNT_CALL_REDIRECT_CONTRACT_BINARY = constructProxy2.apply("016a", "e4cbd3a7");

feat: implement hss sign schedule function

ac810e5

Signed-off-by: Luke Lee <[email protected]>

lukelee-sl added this to the v0.58 milestone Nov 26, 2024

lukelee-sl self-assigned this Nov 26, 2024

lukelee-sl requested review from a team and tinker-michaelj as code owners November 26, 2024 03:51

stoyanov-st previously approved these changes Nov 26, 2024

View reviewed changes

address pr comments

83e1936

Signed-off-by: Luke Lee <[email protected]>

lukelee-sl dismissed stoyanov-st’s stale review via 83e1936 November 26, 2024 17:39

stoyanov-st previously approved these changes Nov 26, 2024

View reviewed changes

david-bakin-sl mentioned this pull request Nov 27, 2024

chore: cherry pick #16789 - Implement hss functions #16803

Open

Merge branch 'develop' of https://github.com/hashgraph/hedera-services …

17dfde9

…into 16700-implement-sign-schedule

david-bakin-sl reviewed Nov 27, 2024

View reviewed changes

lukelee-sl added 2 commits November 27, 2024 13:20

address pr comments 2

7e99e3c

Signed-off-by: Luke Lee <[email protected]>

break up the proxy redirect code in order to clarify

3d87889

Signed-off-by: Luke Lee <[email protected]>

lukelee-sl dismissed stoyanov-st’s stale review via 3d87889 November 27, 2024 21:26

david-bakin-sl approved these changes Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement hss sign schedule function #16789

feat: implement hss sign schedule function #16789

lukelee-sl commented Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

codacy-production bot commented Nov 26, 2024 •

edited

Loading

stoyanov-st left a comment

stoyanov-st left a comment

david-bakin-sl left a comment

david-bakin-sl Nov 27, 2024

david-bakin-sl Nov 27, 2024 •

edited

Loading

david-bakin-sl Nov 27, 2024

lukelee-sl Nov 27, 2024

david-bakin-sl Nov 27, 2024

lukelee-sl Nov 27, 2024

david-bakin-sl Nov 27, 2024

david-bakin-sl Nov 27, 2024

lukelee-sl Nov 27, 2024

david-bakin-sl Nov 27, 2024

lukelee-sl Nov 27, 2024

david-bakin-sl Nov 27, 2024

lukelee-sl Nov 27, 2024

david-bakin-sl left a comment

david-bakin-sl Nov 27, 2024

david-bakin-sl Nov 27, 2024

		assertThat(derivedVerifier.authorizingSimpleKeys()).containsExactly(A_CONTRACT_ID_KEY);
		assertThat(derivedVerifier.authorizingSimpleKeys()).isNotEmpty();

		@@ -91,26 +91,40 @@ public class DispatchingEvmFrameState implements EvmFrameState {

		private static final String ADDRESS_BYTECODE_PATTERN = "fefefefefefefefefefefefefefefefefefefefe";

		private static final String PROXY_PRE_BYTES = "6080604052348015600f57600080fd5b506000610";

feat: implement hss sign schedule function #16789

Are you sure you want to change the base?

feat: implement hss sign schedule function #16789

Conversation

lukelee-sl commented Nov 26, 2024

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

codacy-production bot commented Nov 26, 2024 • edited Loading

Coverage summary from Codacy

stoyanov-st left a comment

Choose a reason for hiding this comment

stoyanov-st left a comment

Choose a reason for hiding this comment

david-bakin-sl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-bakin-sl Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-bakin-sl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2024 •

edited

Loading

codacy-production bot commented Nov 26, 2024 •

edited

Loading

david-bakin-sl Nov 27, 2024 •

edited

Loading