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

DRAFT: Perform Apricot Phase 2 Validation on EVM ImportTxs #187

Merged
merged 23 commits into from
May 7, 2021

Conversation

EvanJRichard
Copy link
Contributor

@EvanJRichard EvanJRichard commented May 5, 2021

Summary

ImportTxs for the EVM now have to obey two new rules:

  • The array of EVMOutputs must not have any duplicate (assetID, address) pairs. That is, there cannot be multiple outputs sending the same asset to the same address. A given address may receive multiple assets, and a given asset may be sent to multiple addresses, but you can't (for example) send my address some AVAX in two separate Outputs.
  • The importing transaction must burn enough AVAX to cover a transaction fee. That is, the difference between AVAX-related Input amounts and AVAX-related EVMOutputs must be equal to or greater than the transaction fee.

At the end of ImportTx construction, validateOuts is called to enforce these rules, throwing errors if they are broken. Because this function needs access to ins and outs, but it is legal to call the constructor without ins and outs, validateOuts is only called if both ins and outs is provided to the constructor.

TODOs/In Progress/Questions

  • Question: proper way to get amount from an input / proper way to get the amount burned
    • There's getBurn in the common/evmtx.ts:EVMStandardTx class, but one doesn't seem to have access to that from the ImportTx constructor. Instead the getBurn code was used as a template for the existing calculation at time of writing.
  • Question: proper way to get the AVAX AssetID
    • the proper way to do this is normally by calling the API, which is outside the scope of the importTx constructor. At time of writing a hardcoded constant is used.
    • A possible solution is to add it as an optional parameter, falling back to the hardcoded constant if not provided
  • Question: proper way to get the fee required
    • the proper way to do this is normally by calling the API, which is outside the scope of the importTx constructor. At time of writing this is retrieved from Defaults.
    • Similar to above, maybe we add it as an optional parameter, falling back to the hardcoded constant if not provided
  • TODO: Fix scripts as needed
    • In the course of attempting to test this, I realized I can't run (for example) buildExportTx-cchain-avax.ts successfully, there's some investigation and bugfixing to do there. EDIT: This ended up being a case of a wrong chainID, which makes me wonder:
  • QUESTION: is a new Defaults.network needed? TODO add it if so

See Also

I wonder if the buildImportTx implementation should change at all, or if that's outside the scope of this PR.

Testing

evm-multi-avax-import-fail

Here’s a script to test a failure scenario. This script creates 2 EVMOutputs w/ the same address and assetId.

ts-node evm-multi-avax-import-fail.ts
EVMOutputError: Error - ImportTx: duplicate (address, assetId) pair found in outputs: (0x8db97c7cece249c2b98bdc0226cc4c2a57bf52fc, 2fombhL7aGPwj3KH4bfrmJwW6PVnMobf9Y2fn9GwxiAAJyFDbe)
errorCode: '1021'

evm-multi-avax-import-success

Here’s a script to test a success scenario. This script creates 2 EVMOutputs w/ the same assetId but different addresses which is valid.

ts-node evm-multi-avax-import-success.ts
Success! TXID: Wg4bmPRoes7nP48BKNnR5bmDfH7R8gTS2wQm7fHec4gFFYE99

evm-multi-asset-import-success

First mint an ANT with this script. Next use this script to test a success scenario. This script creates 2 EVMOutputs for AVAX and 2 EVMOutputs for the newly minted ANT. For each pair of EVMOutputs they have the same assetId but different addresses which is valid

evm-ant-import-fail

First mint an ANT with this script. Next use this script to test a failure scenario. This creates 1 EVMOutput with a non-AVAX assetId and no AVAX fee. This should fail.

ts-node evm-ant-import-fail.ts

EVMFeeError: Error - 1000000 AVAX required for fee and only 0 AVAX provided
errorCode: '1038'

@EvanJRichard EvanJRichard self-assigned this May 5, 2021
@cgcardona cgcardona changed the base branch from master to development May 6, 2021 00:55
* * Add AVAX AssetID for each network as constant
* Formatting
* Return values
* Access  txFee via .network
* Comment.
@EvanJRichard EvanJRichard marked this pull request as ready for review May 6, 2021 17:14
* Check cb58 values instead of hex
* Subtracting fee from EVMImportTx
* Variable names
* * Subtracting fee from EVMImportTx.
* Check cb58 values instead of hex

* * Formatting
* Variable names
* Now that the AVAX AssetId is a constant we can remove these async calls
* Formatting to use fooID instead of fooid or fooId
* * Formatting
* Variable names

* Now that the AVAX AssetId is a constant we can remove this async call.

* Refactoring.

* Formatting.

* More formatting.

* Cleanup.
Tests
* Multi AVAX EVMOutput fail
* Multi ANT EVMOutput fail
* Single AVAX EVMOutput fail
* Single ANT EVMOutput fail
Miscellaneous
* Fix jest warning
* Select networkId
* Use multiple hex addresses
* Formatting and cleanup
Tests:
Single ANT EVMOutput success
Multiple ANT EVMOutput success
Single underfunded AVAX EVMOutput fail
Single AVAX EVMOutput success
Copy link
Contributor

@cgcardona cgcardona left a comment

Choose a reason for hiding this comment

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

Looks good. Let's merge to development. 🎖️

@EvanJRichard EvanJRichard merged commit b9110f1 into development May 7, 2021
@cgcardona cgcardona deleted the evan/AS1004 branch May 7, 2021 20:39
@cgcardona cgcardona mentioned this pull request May 7, 2021
dfenstermaker pushed a commit that referenced this pull request Feb 22, 2023
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants