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.

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

fix: ERC approve and remove scenarios #10325

Merged
merged 35 commits into from
Dec 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
52c5819
test: fix someErc721SetApprovedForAllScenariosPass tests
mustafauzunn Dec 4, 2023
4090097
style: formatting
mustafauzunn Dec 4, 2023
d0c9616
style: formatting
mustafauzunn Dec 4, 2023
f614f9b
refactor: code
mustafauzunn Dec 5, 2023
3024dfe
style: formatting
mustafauzunn Dec 5, 2023
31788f8
style: formatting
mustafauzunn Dec 5, 2023
f98eb01
style: formatting
mustafauzunn Dec 5, 2023
a9aab3a
Merge branch 'develop' of github.com:hashgraph/hedera-services into 1…
mustafauzunn Dec 6, 2023
4c53938
refactor: code
mustafauzunn Dec 6, 2023
bcbb179
refactor: code
mustafauzunn Dec 6, 2023
97da682
refactor: code
mustafauzunn Dec 6, 2023
7eee0ca
test: fix unit tests
mustafauzunn Dec 7, 2023
903bbfd
style: formatting
mustafauzunn Dec 7, 2023
7eb59b3
refactor: code
mustafauzunn Dec 7, 2023
a340df0
test: add nft approval revocation unit tests
mustafauzunn Dec 7, 2023
53a9e9a
style: formatting
mustafauzunn Dec 7, 2023
ac475df
fix: approved for all scenarios
mustafauzunn Dec 11, 2023
439d375
style: formatting
mustafauzunn Dec 11, 2023
883773b
test: fix unit tests
mustafauzunn Dec 11, 2023
ef6422b
refactor: code
mustafauzunn Dec 13, 2023
9799b70
test: fix unit tests
mustafauzunn Dec 13, 2023
8c9dca2
Merge branch 'develop' into 10188-fix-approve-and-remove-scenarios
stoyanov-st Dec 14, 2023
d62dc75
style: formatting
mustafauzunn Dec 14, 2023
e20a431
fix: return failure contract call result if no spender is present
stoyanov-st Dec 15, 2023
f6985c9
test: fix unit tests
mustafauzunn Dec 18, 2023
7ffeee3
refactor: code
mustafauzunn Dec 18, 2023
2577fd9
style: formatting
mustafauzunn Dec 18, 2023
e313819
fix: enable ERC delete allowance with delegated spender
stoyanov-st Dec 18, 2023
dc632bf
Merge branch 'develop' into 10188-fix-approve-and-remove-scenarios
tinker-michaelj Dec 18, 2023
5ee1029
test: add coverage
stoyanov-st Dec 18, 2023
3919cb2
refactor: address PR comments
stoyanov-st Dec 19, 2023
2b5dbec
Merge branch 'develop' into 10188-fix-approve-and-remove-scenarios
stoyanov-st Dec 19, 2023
ccc362e
Merge branch 'develop' into 10188-fix-approve-and-remove-scenarios
tinker-michaelj Dec 19, 2023
2bf5c15
refactor: remove obsolete check
stoyanov-st Dec 20, 2023
0344f31
Merge remote-tracking branch 'origin/10188-fix-approve-and-remove-sce…
stoyanov-st Dec 20, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,16 @@

package com.hedera.node.app.service.contract.impl.exec.systemcontracts.hts.grantapproval;

import static java.util.Objects.requireNonNull;

import com.hedera.hapi.node.base.AccountID;
import com.hedera.hapi.node.base.TokenID;
import com.hedera.hapi.node.base.TokenType;
import com.hedera.hapi.node.state.token.AccountApprovalForAllAllowance;
import com.hedera.hapi.node.token.CryptoApproveAllowanceTransactionBody;
import com.hedera.hapi.node.token.CryptoDeleteAllowanceTransactionBody;
import com.hedera.hapi.node.token.NftAllowance;
import com.hedera.hapi.node.token.NftRemoveAllowance;
import com.hedera.hapi.node.token.TokenAllowance;
import com.hedera.hapi.node.transaction.TransactionBody;
import com.hedera.node.app.service.contract.impl.exec.gas.SystemContractGasCalculator;
Expand All @@ -29,6 +34,7 @@
import com.hedera.node.app.service.contract.impl.hevm.HederaWorldUpdater.Enhancement;
import edu.umd.cs.findbugs.annotations.NonNull;
import java.math.BigInteger;
import java.util.List;

public abstract class AbstractGrantApprovalCall extends AbstractHtsCall {
protected final VerificationStrategy verificationStrategy;
Expand Down Expand Up @@ -60,32 +66,90 @@
}

public TransactionBody callGrantApproval() {
return TransactionBody.newBuilder()
.cryptoApproveAllowance(approve(token, spender, amount, tokenType))
if (tokenType == TokenType.NON_FUNGIBLE_UNIQUE) {
var ownerId = getOwnerId();

if (ownerId != null && !isNftApprovalRevocation()) {
List<AccountApprovalForAllAllowance> accountApprovalForAllAllowances = enhancement
.nativeOperations()
.getAccount(ownerId.accountNum())
.approveForAllNftAllowances();
if (accountApprovalForAllAllowances != null) {
for (var approvedForAll : accountApprovalForAllAllowances) {
if (approvedForAll.tokenId().equals(token)) {
return buildCryptoApproveAllowance(approveDelegate(ownerId, approvedForAll.spenderId()));

Check warning on line 80 in hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hts/grantapproval/AbstractGrantApprovalCall.java

View check run for this annotation

Codecov / codecov/patch

hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hts/grantapproval/AbstractGrantApprovalCall.java#L80

Added line #L80 was not covered by tests
}
}

Check warning on line 82 in hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hts/grantapproval/AbstractGrantApprovalCall.java

View check run for this annotation

Codecov / codecov/patch

hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hts/grantapproval/AbstractGrantApprovalCall.java#L82

Added line #L82 was not covered by tests
}
}

return isNftApprovalRevocation()
? buildCryptoDeleteAllowance(remove(ownerId))
: buildCryptoApproveAllowance(approve(ownerId));
} else {
return buildCryptoApproveAllowance(approve(senderId));
}
}

private CryptoDeleteAllowanceTransactionBody remove(AccountID ownerId) {
return CryptoDeleteAllowanceTransactionBody.newBuilder()
.nftAllowances(NftRemoveAllowance.newBuilder()
.tokenId(token)
.owner(ownerId)
.serialNumbers(amount.longValue())
.build())
.build();
}

private CryptoApproveAllowanceTransactionBody approve(
@NonNull final TokenID token,
@NonNull final AccountID spender,
@NonNull final BigInteger amount,
@NonNull final TokenType tokenType) {
private CryptoApproveAllowanceTransactionBody approveDelegate(AccountID ownerId, AccountID delegateSpenderId) {
return CryptoApproveAllowanceTransactionBody.newBuilder()
.nftAllowances(NftAllowance.newBuilder()
.tokenId(token)
.spender(spender)
.delegatingSpender(delegateSpenderId)
.owner(ownerId)
.serialNumbers(amount.longValue())
.build())
.build();

Check warning on line 113 in hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hts/grantapproval/AbstractGrantApprovalCall.java

View check run for this annotation

Codecov / codecov/patch

hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hts/grantapproval/AbstractGrantApprovalCall.java#L105-L113

Added lines #L105 - L113 were not covered by tests
}

private CryptoApproveAllowanceTransactionBody approve(AccountID ownerId) {
return tokenType.equals(TokenType.FUNGIBLE_COMMON)
? CryptoApproveAllowanceTransactionBody.newBuilder()
.tokenAllowances(TokenAllowance.newBuilder()
.tokenId(token)
.spender(spender)
.owner(senderId)
.owner(ownerId)
.amount(amount.longValue())
.build())
.build()
: CryptoApproveAllowanceTransactionBody.newBuilder()
.nftAllowances(NftAllowance.newBuilder()
.tokenId(token)
.spender(spender)
.owner(senderId)
.owner(ownerId)
.serialNumbers(amount.longValue())
.build())
.build();
}

private TransactionBody buildCryptoDeleteAllowance(CryptoDeleteAllowanceTransactionBody body) {
return TransactionBody.newBuilder().cryptoDeleteAllowance(body).build();
}

private TransactionBody buildCryptoApproveAllowance(CryptoApproveAllowanceTransactionBody body) {
return TransactionBody.newBuilder().cryptoApproveAllowance(body).build();
}

private AccountID getOwnerId() {
final var nft = enhancement.nativeOperations().getNft(token.tokenNum(), amount.longValue());
requireNonNull(nft);
return nft.hasOwnerId()
? nft.ownerId()

Check warning on line 148 in hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hts/grantapproval/AbstractGrantApprovalCall.java

View check run for this annotation

Codecov / codecov/patch

hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hts/grantapproval/AbstractGrantApprovalCall.java#L148

Added line #L148 was not covered by tests
: enhancement.nativeOperations().getToken(token.tokenNum()).treasuryAccountId();
}

private boolean isNftApprovalRevocation() {
return spender.accountNum() == 0;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,14 @@

package com.hedera.node.app.service.contract.impl.exec.systemcontracts.hts.grantapproval;

import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_ALLOWANCE_SPENDER_ID;
import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_TOKEN_ID;
import static com.hedera.node.app.service.contract.impl.exec.systemcontracts.FullResult.revertResult;
import static com.hedera.node.app.service.contract.impl.exec.systemcontracts.FullResult.successResult;
import static com.hedera.node.app.service.contract.impl.exec.systemcontracts.HtsSystemContract.HTS_EVM_ADDRESS;
import static com.hedera.node.app.service.contract.impl.exec.systemcontracts.hts.HtsCall.PricedResult.gasOnly;
import static com.hedera.node.app.service.contract.impl.utils.ConversionUtils.asEvmContractId;
import static com.hedera.node.app.service.contract.impl.utils.SystemContractUtils.contractFunctionResultFailedForProto;

import com.hedera.hapi.node.base.AccountID;
import com.hedera.hapi.node.base.ResponseCodeEnum;
Expand All @@ -28,10 +32,14 @@
import com.hedera.node.app.service.contract.impl.exec.gas.DispatchType;
import com.hedera.node.app.service.contract.impl.exec.gas.SystemContractGasCalculator;
import com.hedera.node.app.service.contract.impl.exec.scope.VerificationStrategy;
import com.hedera.node.app.service.contract.impl.exec.systemcontracts.FullResult;
import com.hedera.node.app.service.contract.impl.exec.systemcontracts.hts.ReturnTypes;
import com.hedera.node.app.service.contract.impl.hevm.HederaWorldUpdater.Enhancement;
import com.hedera.node.app.spi.workflows.record.SingleTransactionRecordBuilder;
import com.hedera.pbj.runtime.io.buffer.Bytes;
import edu.umd.cs.findbugs.annotations.NonNull;
import java.math.BigInteger;
import org.hyperledger.besu.datatypes.Address;

public class ERCGrantApprovalCall extends AbstractGrantApprovalCall {

Expand All @@ -53,7 +61,22 @@ public PricedResult execute() {
if (token == null) {
return reversionWith(INVALID_TOKEN_ID, gasCalculator.canonicalGasRequirement(DispatchType.APPROVE));
}
final var spenderAccount = enhancement.nativeOperations().getAccount(spender.accountNum());
final var body = callGrantApproval();
if (spenderAccount == null && spender.accountNum() != 0) {
var gasRequirement = gasCalculator.canonicalGasRequirement(DispatchType.APPROVE);
var revertResult = FullResult.revertResult(INVALID_ALLOWANCE_SPENDER_ID, gasRequirement);
var result = gasOnly(revertResult, INVALID_ALLOWANCE_SPENDER_ID, false);

var contractID = asEvmContractId(Address.fromHexString(HTS_EVM_ADDRESS));
var encodedRc = ReturnTypes.encodedRc(INVALID_ALLOWANCE_SPENDER_ID).array();
var contractFunctionResult = contractFunctionResultFailedForProto(
gasRequirement, INVALID_ALLOWANCE_SPENDER_ID.protoName(), contractID, Bytes.wrap(encodedRc));

enhancement.systemOperations().externalizeResult(contractFunctionResult, INVALID_ALLOWANCE_SPENDER_ID);

return result;
}
final var recordBuilder = systemContractOperations()
.dispatch(body, verificationStrategy, senderId, SingleTransactionRecordBuilder.class);
final var gasRequirement = gasCalculator.gasRequirement(body, DispatchType.APPROVE, senderId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,29 @@ public static ContractFunctionResult contractFunctionResultFailedFor(
.build();
}

/**
* Create an error contract function result.
*
* @param gasUsed Report the gas used.
* @param errorMsg The error message to report back to the caller.
* @param contractID The contract ID.
* @param contractCallResult Bytes representation of the contract call result error
* @return The created contract function result when for a failed call.
*/
@NonNull
public static ContractFunctionResult contractFunctionResultFailedForProto(
final long gasUsed,
final String errorMsg,
final ContractID contractID,
final com.hedera.pbj.runtime.io.buffer.Bytes contractCallResult) {
return ContractFunctionResult.newBuilder()
.gasUsed(gasUsed)
.contractID(contractID)
.errorMessage(errorMsg)
.contractCallResult(contractCallResult)
.build();
}

private static ContractID contractIdFromEvmAddress(final byte[] bytes) {
return ContractID.newBuilder()
.contractNum(Longs.fromByteArray(Arrays.copyOfRange(bytes, 12, 20)))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,8 @@ public class TestHelpers {
public static final Address OWNER_BESU_ADDRESS = pbjToBesuAddress(OWNER_ADDRESS);
public static final AccountID UNAUTHORIZED_SPENDER_ID =
AccountID.newBuilder().accountNum(999999L).build();
public static final AccountID REVOKE_APPROVAL_SPENDER_ID =
AccountID.newBuilder().accountNum(0L).build();
public static final Bytes UNAUTHORIZED_SPENDER_ADDRESS = Bytes.fromHex("b284224b8b83a724438cc3cc7c0d333a2b6b3222");
public static final com.esaulpaugh.headlong.abi.Address UNAUTHORIZED_SPENDER_HEADLONG_ADDRESS =
asHeadlongAddress(UNAUTHORIZED_SPENDER_ADDRESS.toByteArray());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@

import com.hedera.hapi.node.base.ResponseCodeEnum;
import com.hedera.hapi.node.base.TokenType;
import com.hedera.hapi.node.state.token.Account;
import com.hedera.hapi.node.state.token.Nft;
import com.hedera.hapi.node.state.token.Token;
import com.hedera.node.app.service.contract.impl.exec.gas.SystemContractGasCalculator;
import com.hedera.node.app.service.contract.impl.exec.scope.VerificationStrategy;
import com.hedera.node.app.service.contract.impl.exec.systemcontracts.hts.grantapproval.ClassicGrantApprovalCall;
Expand All @@ -51,6 +54,15 @@ public class ClassicGrantApprovalCallTest extends HtsCallTestBase {
@Mock
private SystemContractGasCalculator systemContractGasCalculator;

@Mock
private Nft nft;

@Mock
private Token token;

@Mock
private Account account;

@Test
void fungibleApprove() {
subject = new ClassicGrantApprovalCall(
Expand Down Expand Up @@ -87,6 +99,10 @@ void nftApprove() {
TokenType.NON_FUNGIBLE_UNIQUE);
given(systemContractOperations.dispatch(any(), any(), any(), any())).willReturn(recordBuilder);
given(recordBuilder.status()).willReturn(ResponseCodeEnum.SUCCESS);
given(nativeOperations.getNft(NON_FUNGIBLE_TOKEN_ID.tokenNum(), 100L)).willReturn(nft);
given(nativeOperations.getToken(NON_FUNGIBLE_TOKEN_ID.tokenNum())).willReturn(token);
given(token.treasuryAccountId()).willReturn(OWNER_ID);
given(nativeOperations.getAccount(OWNER_ID.accountNum())).willReturn(account);
final var result = subject.execute(frame).fullResult().result();

assertEquals(MessageFrame.State.COMPLETED_SUCCESS, result.getState());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,20 @@
import static com.hedera.node.app.service.contract.impl.test.TestHelpers.FUNGIBLE_TOKEN_ID;
import static com.hedera.node.app.service.contract.impl.test.TestHelpers.NON_FUNGIBLE_TOKEN_ID;
import static com.hedera.node.app.service.contract.impl.test.TestHelpers.OWNER_ID;
import static com.hedera.node.app.service.contract.impl.test.TestHelpers.REVOKE_APPROVAL_SPENDER_ID;
import static com.hedera.node.app.service.contract.impl.test.TestHelpers.UNAUTHORIZED_SPENDER_ID;
import static com.hedera.node.app.service.contract.impl.test.TestHelpers.asBytesResult;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.given;

import com.hedera.hapi.node.base.ResponseCodeEnum;
import com.hedera.hapi.node.base.TokenType;
import com.hedera.hapi.node.state.token.Account;
import com.hedera.hapi.node.state.token.Nft;
import com.hedera.hapi.node.state.token.Token;
import com.hedera.hapi.node.transaction.TransactionBody;
import com.hedera.node.app.service.contract.impl.exec.gas.SystemContractGasCalculator;
import com.hedera.node.app.service.contract.impl.exec.scope.VerificationStrategy;
Expand All @@ -37,7 +42,9 @@
import com.hedera.node.app.service.token.records.CryptoTransferRecordBuilder;
import com.hedera.node.app.spi.workflows.record.SingleTransactionRecordBuilder;
import java.math.BigInteger;
import org.apache.tuweni.bytes.Bytes;
import org.hyperledger.besu.evm.frame.MessageFrame;
import org.hyperledger.besu.evm.frame.MessageFrame.State;
import org.junit.jupiter.api.Test;
import org.mockito.Mock;

Expand All @@ -54,6 +61,15 @@ class ERCGrantApprovalCallTest extends HtsCallTestBase {
@Mock
private CryptoTransferRecordBuilder recordBuilder;

@Mock
private Nft nft;

@Mock
private Token token;

@Mock
private Account account;

@Test
void erc20approve() {
subject = new ERCGrantApprovalCall(
Expand All @@ -72,6 +88,7 @@ void erc20approve() {
eq(SingleTransactionRecordBuilder.class)))
.willReturn(recordBuilder);
given(recordBuilder.status()).willReturn(ResponseCodeEnum.SUCCESS);
given(nativeOperations.getAccount(anyLong())).willReturn(account);
final var result = subject.execute().fullResult().result();

assertEquals(MessageFrame.State.COMPLETED_SUCCESS, result.getState());
Expand Down Expand Up @@ -99,6 +116,67 @@ void erc721approve() {
eq(SingleTransactionRecordBuilder.class)))
.willReturn(recordBuilder);
given(recordBuilder.status()).willReturn(ResponseCodeEnum.SUCCESS);
given(nativeOperations.getNft(NON_FUNGIBLE_TOKEN_ID.tokenNum(), 100L)).willReturn(nft);
given(nativeOperations.getToken(NON_FUNGIBLE_TOKEN_ID.tokenNum())).willReturn(token);
given(token.treasuryAccountId()).willReturn(OWNER_ID);
given(nativeOperations.getAccount(anyLong())).willReturn(account);
final var result = subject.execute().fullResult().result();

assertEquals(MessageFrame.State.COMPLETED_SUCCESS, result.getState());
assertEquals(
asBytesResult(GrantApprovalTranslator.ERC_GRANT_APPROVAL_NFT
.getOutputs()
.encodeElements()),
result.getOutput());
}

@Test
void erc721approveFailsWithInvalidSpenderAllowance() {
subject = new ERCGrantApprovalCall(
mockEnhancement(),
systemContractGasCalculator,
verificationStrategy,
OWNER_ID,
NON_FUNGIBLE_TOKEN_ID,
UNAUTHORIZED_SPENDER_ID,
BigInteger.valueOf(100L),
TokenType.NON_FUNGIBLE_UNIQUE);
given(nativeOperations.getNft(NON_FUNGIBLE_TOKEN_ID.tokenNum(), 100L)).willReturn(nft);
given(nativeOperations.getToken(NON_FUNGIBLE_TOKEN_ID.tokenNum())).willReturn(token);
given(token.treasuryAccountId()).willReturn(OWNER_ID);
given(nativeOperations.getAccount(anyLong())).willReturn(null).willReturn(account);
final var result = subject.execute().fullResult().result();

assertEquals(State.REVERT, result.getState());
assertEquals(
Bytes.wrap(ResponseCodeEnum.INVALID_ALLOWANCE_SPENDER_ID
.protoName()
.getBytes()),
result.getOutput());
}

@Test
void erc721revoke() {
subject = new ERCGrantApprovalCall(
mockEnhancement(),
systemContractGasCalculator,
verificationStrategy,
OWNER_ID,
NON_FUNGIBLE_TOKEN_ID,
REVOKE_APPROVAL_SPENDER_ID,
BigInteger.valueOf(100L),
TokenType.NON_FUNGIBLE_UNIQUE);
given(systemContractOperations.dispatch(
any(TransactionBody.class),
eq(verificationStrategy),
eq(OWNER_ID),
eq(SingleTransactionRecordBuilder.class)))
.willReturn(recordBuilder);
given(recordBuilder.status()).willReturn(ResponseCodeEnum.SUCCESS);
given(nativeOperations.getNft(NON_FUNGIBLE_TOKEN_ID.tokenNum(), 100L)).willReturn(nft);
given(nativeOperations.getToken(NON_FUNGIBLE_TOKEN_ID.tokenNum())).willReturn(token);
given(nativeOperations.getAccount(anyLong())).willReturn(account);
given(token.treasuryAccountId()).willReturn(OWNER_ID);
final var result = subject.execute().fullResult().result();

assertEquals(MessageFrame.State.COMPLETED_SUCCESS, result.getState());
Expand Down