Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: refactor and simplify HIP-540 implementation and tests #13012

Merged
merged 16 commits into from
May 6, 2024
Merged

fix: refactor and simplify HIP-540 implementation and tests #13012

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a6f2a14
Refactor HIP-540
tinker-michaelj Apr 25, 2024
1eb982d
Add query validations
tinker-michaelj Apr 25, 2024
42dfa9e
Remove redundant tests
tinker-michaelj Apr 25, 2024
c3d0278
Resolve merge conflicts
tinker-michaelj Apr 25, 2024
949a038
Add javadoc
tinker-michaelj Apr 25, 2024
69c2895
Allow parallel scenario execution by salting registry names
tinker-michaelj Apr 25, 2024
f63144f
spotless
tinker-michaelj Apr 26, 2024
d360060
Wording
tinker-michaelj Apr 26, 2024
4629ad2
Cleanup
tinker-michaelj Apr 26, 2024
9858421
fix unit test
tinker-michaelj Apr 26, 2024
e50fe29
Prioritize TOKEN_IS_IMMUTABLE when token has neither role nor admin key
tinker-michaelj Apr 26, 2024
429ee9c
Merge remote-tracking branch 'origin/develop' into refactor-hip540
tinker-michaelj Apr 27, 2024
091cf33
Permit updating metadata with just admin key
tinker-michaelj Apr 27, 2024
15b03eb
Merge branch 'hip540-full-implementation' into refactor-hip540
Neeharika-Sompalli Apr 27, 2024
b89672d
some docs
Neeharika-Sompalli Apr 27, 2024
b6dc987
add testName
Neeharika-Sompalli Apr 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions hedera-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/key/KeyUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ private KeyUtils() {
throw new UnsupportedOperationException("Utility Class");
}

public static boolean isEmptyAndNotImmutable(@Nullable final Key pbjKey) {
return isEmptyInternal(pbjKey, true);
}

/**
* Checks if the given key is empty.
* For a KeyList type checks if the list is empty.
Expand Down
30 changes: 15 additions & 15 deletions ...ervice/src/test/java/com/hedera/node/app/service/mono/sigs/order/SigRequirementsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,13 +259,13 @@
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_REPLACING_TREASURY_AS_CUSTOM_PAYER;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_REPLACING_TREASURY_AS_PAYER;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_REPLACING_WITH_MISSING_TREASURY;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_FREEZE_KEYED_TOKEN;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_KYC_KEYED_TOKEN;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_FREEZE_KEYED_TOKEN_NO_VALIDATION;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_KYC_KEYED_TOKEN_NO_VALIDATION;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_MISSING_TOKEN;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_MISSING_TOKEN_ADMIN_KEY;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_NO_KEYS_AFFECTED;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_SUPPLY_KEYED_TOKEN;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_WIPE_KEYED_TOKEN;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_NO_FIELDS_CHANGED;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_SUPPLY_KEYED_TOKEN_NO_VALIDATION;
import static com.hedera.test.factories.scenarios.TokenUpdateScenarios.UPDATE_WITH_WIPE_KEYED_TOKEN_NO_KEY_VALIDATION;
import static com.hedera.test.factories.scenarios.TokenWipeScenarios.VALID_WIPE_WITH_EXTANT_TOKEN;
import static com.hedera.test.factories.scenarios.TxnHandlingScenario.CURRENTLY_UNUSED_ALIAS;
import static com.hedera.test.factories.scenarios.TxnHandlingScenario.CUSTOM_PAYER_ACCOUNT;
Expand Down Expand Up @@ -4567,7 +4567,7 @@ void getsTokenWipeWithRelevantKey() throws Throwable {
@Test
void getsUpdateNoSpecialKeys() throws Throwable {
// given:
setupFor(UPDATE_WITH_NO_KEYS_AFFECTED);
setupFor(UPDATE_WITH_NO_FIELDS_CHANGED);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory);
Expand All @@ -4580,7 +4580,7 @@ void getsUpdateNoSpecialKeys() throws Throwable {
@Test
void getsUpdateNoSpecialKeysWithCustomPayer() throws Throwable {
// given:
setupFor(UPDATE_WITH_NO_KEYS_AFFECTED);
setupFor(UPDATE_WITH_NO_FIELDS_CHANGED);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory, null, CUSTOM_PAYER_ACCOUNT);
Expand All @@ -4593,7 +4593,7 @@ void getsUpdateNoSpecialKeysWithCustomPayer() throws Throwable {
@Test
void getsUpdateWithWipe() throws Throwable {
// given:
setupFor(UPDATE_WITH_WIPE_KEYED_TOKEN);
setupFor(UPDATE_WITH_WIPE_KEYED_TOKEN_NO_KEY_VALIDATION);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory);
Expand All @@ -4606,7 +4606,7 @@ void getsUpdateWithWipe() throws Throwable {
@Test
void getsUpdateWithWipeWithCustomPayer() throws Throwable {
// given:
setupFor(UPDATE_WITH_WIPE_KEYED_TOKEN);
setupFor(UPDATE_WITH_WIPE_KEYED_TOKEN_NO_KEY_VALIDATION);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory, null, CUSTOM_PAYER_ACCOUNT);
Expand All @@ -4619,7 +4619,7 @@ void getsUpdateWithWipeWithCustomPayer() throws Throwable {
@Test
void getsUpdateWithSupply() throws Throwable {
// given:
setupFor(UPDATE_WITH_SUPPLY_KEYED_TOKEN);
setupFor(UPDATE_WITH_SUPPLY_KEYED_TOKEN_NO_VALIDATION);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory);
Expand All @@ -4632,7 +4632,7 @@ void getsUpdateWithSupply() throws Throwable {
@Test
void getsUpdateWithSupplyWithCustomPayer() throws Throwable {
// given:
setupFor(UPDATE_WITH_SUPPLY_KEYED_TOKEN);
setupFor(UPDATE_WITH_SUPPLY_KEYED_TOKEN_NO_VALIDATION);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory, null, CUSTOM_PAYER_ACCOUNT);
Expand All @@ -4645,7 +4645,7 @@ void getsUpdateWithSupplyWithCustomPayer() throws Throwable {
@Test
void getsUpdateWithKyc() throws Throwable {
// given:
setupFor(UPDATE_WITH_KYC_KEYED_TOKEN);
setupFor(UPDATE_WITH_KYC_KEYED_TOKEN_NO_VALIDATION);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory);
Expand All @@ -4658,7 +4658,7 @@ void getsUpdateWithKyc() throws Throwable {
@Test
void getsUpdateWithKycWithCustomPayer() throws Throwable {
// given:
setupFor(UPDATE_WITH_KYC_KEYED_TOKEN);
setupFor(UPDATE_WITH_KYC_KEYED_TOKEN_NO_VALIDATION);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory, null, CUSTOM_PAYER_ACCOUNT);
Expand Down Expand Up @@ -4780,7 +4780,7 @@ void getsUpdateWithNewTreasuryAsPayerWithCustomCustomPayer() throws Throwable {
@Test
void getsUpdateWithFreeze() throws Throwable {
// given:
setupFor(UPDATE_WITH_FREEZE_KEYED_TOKEN);
setupFor(UPDATE_WITH_FREEZE_KEYED_TOKEN_NO_VALIDATION);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory);
Expand All @@ -4793,7 +4793,7 @@ void getsUpdateWithFreeze() throws Throwable {
@Test
void getsUpdateWithFreezeWithCustomPayer() throws Throwable {
// given:
setupFor(UPDATE_WITH_FREEZE_KEYED_TOKEN);
setupFor(UPDATE_WITH_FREEZE_KEYED_TOKEN_NO_VALIDATION);

// when:
var summary = subject.keysForOtherParties(txn, summaryFactory, null, CUSTOM_PAYER_ACCOUNT);
Expand Down
25 changes: 20 additions & 5 deletions ...rvice/src/testFixtures/java/com/hedera/test/factories/scenarios/TokenUpdateScenarios.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import java.security.SignatureException;

public enum TokenUpdateScenarios implements TxnHandlingScenario {
UPDATE_WITH_NO_KEYS_AFFECTED {
UPDATE_WITH_NO_FIELDS_CHANGED {
@Override
public PlatformTxnAccessor platformTxn()
throws InvalidProtocolBufferException, SignatureException, NoSuchAlgorithmException,
Expand Down Expand Up @@ -90,18 +90,19 @@ public PlatformTxnAccessor platformTxn()
.get());
}
},
UPDATE_WITH_SUPPLY_KEYED_TOKEN {
UPDATE_WITH_SUPPLY_KEYED_TOKEN_NO_VALIDATION {
@Override
public PlatformTxnAccessor platformTxn()
throws InvalidProtocolBufferException, SignatureException, NoSuchAlgorithmException,
InvalidKeyException {
return PlatformTxnAccessor.from(TokenUpdateFactory.newSignedTokenUpdate()
.updating(KNOWN_TOKEN_WITH_SUPPLY)
.replacingSupply()
.notValidatingRoleKeySignatures()
.get());
}
},
UPDATE_WITH_KYC_KEYED_TOKEN {
UPDATE_WITH_KYC_KEYED_TOKEN_FULL_VALIDATION {
@Override
public PlatformTxnAccessor platformTxn()
throws InvalidProtocolBufferException, SignatureException, NoSuchAlgorithmException,
Expand All @@ -112,25 +113,39 @@ public PlatformTxnAccessor platformTxn()
.get());
}
},
UPDATE_WITH_FREEZE_KEYED_TOKEN {
UPDATE_WITH_KYC_KEYED_TOKEN_NO_VALIDATION {
@Override
public PlatformTxnAccessor platformTxn()
throws InvalidProtocolBufferException, SignatureException, NoSuchAlgorithmException,
InvalidKeyException {
return PlatformTxnAccessor.from(TokenUpdateFactory.newSignedTokenUpdate()
.updating(KNOWN_TOKEN_WITH_KYC)
.replacingKyc()
.notValidatingRoleKeySignatures()
.get());
}
},
UPDATE_WITH_FREEZE_KEYED_TOKEN_NO_VALIDATION {
@Override
public PlatformTxnAccessor platformTxn()
throws InvalidProtocolBufferException, SignatureException, NoSuchAlgorithmException,
InvalidKeyException {
return PlatformTxnAccessor.from(TokenUpdateFactory.newSignedTokenUpdate()
.updating(KNOWN_TOKEN_WITH_FREEZE)
.replacingFreeze()
.notValidatingRoleKeySignatures()
.get());
}
},
UPDATE_WITH_WIPE_KEYED_TOKEN {
UPDATE_WITH_WIPE_KEYED_TOKEN_NO_KEY_VALIDATION {
@Override
public PlatformTxnAccessor platformTxn()
throws InvalidProtocolBufferException, SignatureException, NoSuchAlgorithmException,
InvalidKeyException {
return PlatformTxnAccessor.from(TokenUpdateFactory.newSignedTokenUpdate()
.updating(KNOWN_TOKEN_WITH_WIPE)
.replacingWipe()
.notValidatingRoleKeySignatures()
.get());
}
},
Expand Down
10 changes: 9 additions & 1 deletion ...mono-service/src/testFixtures/java/com/hedera/test/factories/txns/TokenUpdateFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.hedera.test.factories.keys.KeyTree;
import com.hederahashgraph.api.proto.java.AccountID;
import com.hederahashgraph.api.proto.java.TokenID;
import com.hederahashgraph.api.proto.java.TokenKeyValidation;
import com.hederahashgraph.api.proto.java.TokenUpdateTransactionBody;
import com.hederahashgraph.api.proto.java.Transaction;
import com.hederahashgraph.api.proto.java.TransactionBody;
Expand All @@ -32,6 +33,7 @@ public class TokenUpdateFactory extends SignedTxnFactory<TokenUpdateFactory> {
private Optional<AccountID> newTreasury = Optional.empty();
private Optional<AccountID> newAutoRenew = Optional.empty();
private boolean replaceFreeze, replaceSupply, replaceWipe, replaceKyc;
private TokenKeyValidation keyVerificationMode = TokenKeyValidation.FULL_VALIDATION;

private TokenUpdateFactory() {}

Expand All @@ -44,6 +46,11 @@ public TokenUpdateFactory updating(final TokenID id) {
return this;
}

public TokenUpdateFactory notValidatingRoleKeySignatures() {
this.keyVerificationMode = TokenKeyValidation.NO_VALIDATION;
return this;
}

public TokenUpdateFactory newAdmin(final KeyTree kt) {
newAdminKt = Optional.of(kt);
return this;
Expand Down Expand Up @@ -106,8 +113,9 @@ protected void customizeTxn(final TransactionBody.Builder txn) {
if (replaceWipe) {
op.setWipeKey(TOKEN_REPLACE_KT.asKey());
}
newAutoRenew.ifPresent(a -> op.setAutoRenewAccount(a));
newAutoRenew.ifPresent(op::setAutoRenewAccount);
newTreasury.ifPresent(op::setTreasury);
op.setKeyVerificationMode(keyVerificationMode);
txn.setTokenUpdate(op);
}
}
37 changes: 10 additions & 27 deletions ...-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/BaseTokenHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
import com.hedera.node.app.service.token.impl.WritableAccountStore;
import com.hedera.node.app.service.token.impl.WritableTokenRelationStore;
import com.hedera.node.app.service.token.impl.WritableTokenStore;
import com.hedera.node.app.service.token.impl.util.TokenKeys;
import com.hedera.node.app.service.token.impl.util.TokenKey;
import com.hedera.node.config.data.EntitiesConfig;
import com.hedera.node.config.data.TokensConfig;
import com.swirlds.config.api.Configuration;
Expand All @@ -58,8 +58,8 @@

public class BaseTokenHandler {
private static final Logger log = LogManager.getLogger(BaseTokenHandler.class);
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think TOKEN_KEYS and NON_ADMIN_TOKEN_KEYS can stay protected


/**
* Mints fungible tokens. This method is called in both token create and mint.
Expand Down Expand Up @@ -429,31 +429,14 @@ public static boolean isExpiryOnlyUpdateOp(@NonNull final TokenUpdateTransaction
}

/**
* Check if TokenUpdateOp wants to update only some of the low priority keys or the metadata field
* low priority keys are -> wipeKey, kycKey, supplyKey, freezeKey, feeScheduleKey, pauseKey or metadataKey
*/
public static boolean noOtherFieldThanLowPriorityKeyOrMetadataWillBeUpdated(
@NonNull final TokenUpdateTransactionBody op) {
return !((op.symbol() != null && !op.symbol().isEmpty())
|| (op.name() != null && !op.name().isEmpty())
|| op.hasTreasury()
|| op.hasAdminKey()
|| op.hasAutoRenewAccount()
|| op.hasAutoRenewPeriod()
|| op.hasMemo()
|| op.hasExpiry());
}

/**
* Check if a given token already has some of the low priority keys
* Checks if the given op updates any of the non-key token properties that can only be
* changed given the admin key signature.
*
* @param op the token update op to check
* @return true if it requires admin key signature
*/
public static boolean hasAlreadySomeNonAdminKeys(@NonNull final Token token) {
for (final var nonAdminKey : NON_ADMIN_TOKEN_KEYS) {
if (nonAdminKey.isPresentInitially(token)) {
return true;
}
}
return false;
protected static boolean updatesAdminOnlyNonKeyTokenProperty(@NonNull final TokenUpdateTransactionBody op) {
return !op.symbol().isEmpty() || !op.name().isEmpty() || op.hasAutoRenewPeriod() || op.hasMemo();
Neeharika-Sompalli marked this conversation as resolved.
Show resolved Hide resolved
}

@NonNull
Expand Down