fix: refactor and simplify HIP-540 implementation and tests #13012
Conversation
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Node: HAPI Test (Node Death Reconnect) Results1 tests ±0 1 ✅ ±0 22s ⏱️ -5s For more details on these parsing errors, see this check. Results for commit b6dc987. ± Comparison against base commit 818f784. ♻️ This comment has been updated with latest results. |
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Node: HAPI Test (Misc) Results457 tests +457 447 ✅ +447 42m 15s ⏱️ + 42m 15s For more details on these parsing errors, see this check. Results for commit b6dc987. ± Comparison against base commit 818f784. ♻️ This comment has been updated with latest results. |
Node: HAPI Test (Smart Contract) Results583 tests ±0 582 ✅ ±0 1h 12m 48s ⏱️ + 10m 27s For more details on these failures, see this check. Results for commit b6dc987. ± Comparison against base commit 818f784. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
I totally agree with enhancing the pureChecks() and removing some unused methods as well as requireNonNull() and OrThrow() assertions, however why we remove Hip540ChangeOrRemoveKeysSuite. The file contains lots of corner cases to test every combination of TokenUpdateTx to examine every possible behaviour for this HIP.
With the provided changes in this PR, 17 from 29 tests in Hip540ChangeOrRemoveKeysSuite are failing as well as some of the already existing metadata HIP tests
| for (final var tokenKey : TOKEN_KEYS) { | ||
| if (tokenKey.isPresentInUpdate(op)) { | ||
| final var key = tokenKey.getFromUpdate(op); | ||
| if (!isKeyRemoval(key)) { |
There was a problem hiding this comment.
| if (!isKeyRemoval(key)) { | |
| if (!isKeyRemoval(key) && op.keyVerificationMode() != TokenKeyValidation.NO_VALIDATION) { |
if we do not check for the TokenKeyValidation enum, a user will always end up with INVALID_<SOME>_KEY in prechecks when he tries to update to an invalid key. IMO, we should check if the TokenKeyValidation is NO_VALIDATION and in this case to not throw anything and allow the user to update to an invalid key (part of the HIP requirements)
There was a problem hiding this comment.
I have opened a HIP PR to eliminate misleading use of the term "invalid"; we definitely don't want to introduce a way to put invalid key structures in state.
| */ | ||
| private void requireAdmin(@NonNull final PreHandleContext context, @NonNull final Token originalToken) | ||
| throws PreCheckException { | ||
| validateTruePreCheck(originalToken.hasAdminKey(), TOKEN_IS_IMMUTABLE); |
There was a problem hiding this comment.
Previously, we were throwing TOKEN_IS_IMMUTABLE when the token cannot be updated under any circumstances. With this logic, we will throw it if the admin key is required for the success of the update transaction and it is not present. But this does not mean that the token is immutable. If the token has any low priority key, it can still be updated if for example the freeze key want to update itself (part of the HIP requirements)
There was a problem hiding this comment.
It would be nice to have more targeted response codes for the specific failure of trying to change a non-key, non-metadata property without an admin key, true...in general, we need richer failure statuses. 🤔
| protected static final Set<TokenKeys> TOKEN_KEYS = EnumSet.allOf(TokenKeys.class); | ||
| protected static final Set<TokenKeys> NON_ADMIN_TOKEN_KEYS = EnumSet.complementOf(EnumSet.of(TokenKeys.ADMIN_KEY)); | ||
| public static final Set<TokenKey> NON_ADMIN_TOKEN_KEYS = EnumSet.complementOf(EnumSet.of(TokenKey.ADMIN_KEY)); | ||
| static final Set<TokenKey> TOKEN_KEYS = EnumSet.allOf(TokenKey.class); |
There was a problem hiding this comment.
I think TOKEN_KEYS and NON_ADMIN_TOKEN_KEYS can stay protected
| } else { | ||
| context.requireKey(maybeRoleKey); | ||
| if (replacementKey != null) { | ||
| context.requireKey(replacementKey); |
There was a problem hiding this comment.
Whenever a low priority key wants to update itself to other key. In this case, the replacementKey is not required to sign the transaction
i.e: kycKey wants to update itself to newKycKey -> we require only kycKey signature (part of the HIP requirements)
There was a problem hiding this comment.
I have updated the HIP to clarify that without the admin signature, the new low-privilege key also needs to sign.
(This is the only meaningful interpretation of TokenKeyValidation.NONE, I think.)
There was a problem hiding this comment.
I see, this changes the initial requirements quite a bit
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
...ice-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/BaseTokenHandler.java
Show resolved
Hide resolved
tinker-michaelj
requested review from
matteriben,
nathanklick,
edward-swirldslabs,
hendrikebbers,
imalygin and
OlegMazurov
Neeharika-Sompalli
requested review from
netopyr
and removed request for
a team,
kimbor,
imalygin,
rbair23,
nathanklick,
hendrikebbers,
OlegMazurov,
david-bakin-sl,
edward-swirldslabs and
matteriben
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
tinker-michaelj
merged commit 279b7ae
into
hip540-full-implementation
Description:
TOKEN_UPDATEchecks intopureChecks().Hip540TestScenariovalue that is capable of translating itself into a sequence ofHapiSpecOperations; and repeat all these scenarios for each non-admin key type here.requireNonNull()andOrThrow()assertions to fail faster in some cases.