Skip to content
This repository has been archived by the owner on Mar 24, 2023. It is now read-only.

Support native transfer #505

Merged
merged 5 commits into from
Sep 2, 2022
Merged

Conversation

keroro520
Copy link
Contributor

@keroro520 keroro520 commented Aug 25, 2022

Web3 supports native transfer

When sending a native transfer transaction, appends to_address to polyjuice_args, uses POLYJUICE_CREATOR_ACCOUNT_ID as to_id;
when getting a native transfer transaction, extract to_address from polyjuice_args.

Indexer supports native transfer

For a native transfer transaction, extract to_address from polyjuice_args.

Related PRs

@keroro520 keroro520 self-assigned this Aug 25, 2022
@keroro520 keroro520 force-pushed the native-transfer branch 2 times, most recently from 39cd0fa to b38ab13 Compare August 25, 2022 08:41
@RetricSu
Copy link
Contributor

pls update https://github.com/nervosnetwork/godwoken-web3/blob/main/docs/compatibility.md#1-transactionto-must-be-a-contract-address as well

@RetricSu
Copy link
Contributor

so right now eth_sendRawTransaction is good with EOA to address, what about eth_call and eth_estimateGas? Can they work with EOA to address now?

@keroro520
Copy link
Contributor Author

so right now eth_sendRawTransaction is good with EOA to address, what about eth_call and eth_estimateGas? Can they work with EOA to address now?

The approach Web3 determines whether the transaction is native transafer tx: https://github.com/nervosnetwork/godwoken-web3/pull/505/files#diff-9b0dee551144978abbf32f56e4753f613c673ca1fce488d1d3f754556500f399R486-R500

/**
 * Determine whether the transaction is a native transfer transaction.
 *
 * When tx.to refers to an EOA account or an account that doesn't exist, it is considered a native transfer transaction.
 */
async function isEthNativeTransfer(
  { to: toAddress }: { to: HexString },
  rpc: GodwokenClient
): Promise<boolean> {
  if (toAddress.length === 42) {
    const toId = await getAccountIdByEthAddress(toAddress, rpc);
    return toId == null || isEthEOA(toAddress, toId, rpc);
  }
  return false;
}

For a tx that calling existed contract, the tx.to points to a contract, so isEthNativeTransfer returns false;
For a tx that calling nonexistant contract, the tx.to points to null, so isEthNativeTransfer returns true.

I have not test "eth_call and eth_estimateGas with EOA to address". In my opinion, if tx.to is EOA or null, it should be a native transfer tx, but not "call".

@RetricSu
Copy link
Contributor

RetricSu commented Aug 29, 2022

yes, but both eth_call and eth_estimateGas RPCs allow the to address to be an EOA address in Ethereum. Previously we reject such tx so it is incompatible, I want to make sure what is the behavior right now. I believe such tests are necessary. and it will be better if we can make it compatible.

@keroro520
Copy link
Contributor Author

yes, but both eth_call and eth_estimateGas RPCs allow the to address to be an EOA address in Ethereum. Previously we reject such tx so it is incompatible, I want to make sure what is the behavior right now. I believe such tests are necessary. and it will be better if we can make it compatible.

Yes, we should use tests to enforce the compatibility. Later, I'll test it manually and give you feedback.
And I know @Dawn-githup is planning full test cases. cc @Dawn-githup

.github/workflows/godwoken-tests.yml Outdated Show resolved Hide resolved
packages/api-server/src/filter-web3-tx.ts Outdated Show resolved Hide resolved
crates/indexer/src/indexer.rs Outdated Show resolved Hide resolved
@keroro520
Copy link
Contributor Author

@keroro520
Copy link
Contributor Author

yes, but both eth_call and eth_estimateGas RPCs allow the to address to be an EOA address in Ethereum. Previously we reject such tx so it is incompatible, I want to make sure what is the behavior right now. I believe such tests are necessary. and it will be better if we can make it compatible.

Yes, we should use tests to enforce the compatibility. Later, I'll test it manually and give you feedback. And I know @Dawn-githup is planning full test cases. cc @Dawn-githup

Success to transfer to a payable contact.

.github/workflows/godwoken-tests.yml Outdated Show resolved Hide resolved
@keroro520 keroro520 force-pushed the native-transfer branch 2 times, most recently from 146e9a8 to 0c4ddf8 Compare September 1, 2022 09:27
@keroro520
Copy link
Contributor Author

Update:

  • The new commit 2c076ad allows eth_call/eth_estimateGas accept a native transfer tx. cc @RetricSu

packages/api-server/src/convert-tx.ts Outdated Show resolved Hide resolved
packages/api-server/src/convert-tx.ts Outdated Show resolved Hide resolved
packages/api-server/src/convert-tx.ts Outdated Show resolved Hide resolved
packages/api-server/src/convert-tx.ts Outdated Show resolved Hide resolved
packages/api-server/src/convert-tx.ts Outdated Show resolved Hide resolved
packages/api-server/src/convert-tx.ts Outdated Show resolved Hide resolved
packages/api-server/src/convert-tx.ts Outdated Show resolved Hide resolved
packages/api-server/src/convert-tx.ts Show resolved Hide resolved
packages/api-server/src/methods/modules/eth.ts Outdated Show resolved Hide resolved
packages/api-server/src/parse-tx.ts Outdated Show resolved Hide resolved
.github/workflows/godwoken-tests.yml Outdated Show resolved Hide resolved
crates/indexer/src/helper.rs Outdated Show resolved Hide resolved
crates/indexer/src/indexer.rs Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants