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

[Breaking] Refactor Ethereum token transfer signing input #1210

Merged
merged 44 commits into from
Dec 21, 2020

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Dec 8, 2020

Description

BREAKING change. Change in Eth signing input message, introducing specialized messages for different transfers:
native coin, ERC20, generic smart contract, etc.
Fixes #1176 .

Breaking Change

SigningInput for ETH transfer: The amount field is now moved to sub-message named contractTransfer of type TransferContract. This is a breaking change!
Pay attention especially to JSON version, as the breaking change will not come out at compile time (signJSON()).

SigningInput for ERC20 transfer: This is a new addition; sub-message named contractErc20 of type ERC20TransferContract should be used, with members toAddress and amount. Building of the smart contract call is done by wallet-core, not the caller/app.

SigningInput for smart contract call: This is a new addition; for smart contract calls (except the special case of ERC20 above) a new sub-message named contractGeneric of type GenericContract should be used, with member payload.

See files for examples:

  • src/proto/Ethereum.proto
  • swift/Tests/Blockchains/EthereumTests.swift
  • android/app/src/androidTest/java/com/trustwallet/core/app/blockchains/ethereum/TestEthereumTransactionSigner.kt
  • tests/Ethereum/TWAnySignerTests.cpp
  • tests/Ethereum/SignerTests.cpp

Testing instructions

Unit tests, esp. iOS&Android.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • I have read the guidelines for adding a new blockchain
  • If there is a related Issue, mention it in the description (e.g. Fixes #<issue_number> ).

@optout21 optout21 marked this pull request as draft December 8, 2020 10:24
@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #1210 (2647c6d) into master (61091e0) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1210      +/-   ##
==========================================
+ Coverage   95.24%   95.27%   +0.03%     
==========================================
  Files         432      433       +1     
  Lines       12968    13034      +66     
==========================================
+ Hits        12351    12418      +67     
+ Misses        617      616       -1     
Impacted Files Coverage Δ
src/Ethereum/Signer.cpp 99.02% <100.00%> (+0.49%) ⬆️
src/Ethereum/Transaction.cpp 100.00% <100.00%> (ø)
src/Ethereum/Transaction.h 100.00% <100.00%> (ø)
src/IoTeX/Staking.cpp 100.00% <100.00%> (ø)
src/Solana/Transaction.cpp 100.00% <100.00%> (ø)
src/Ethereum/Address.cpp 90.47% <0.00%> (+4.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35a6307...2647c6d. Read the comment docs.

src/proto/Ethereum.proto Outdated Show resolved Hide resolved
src/Ethereum/Signer.cpp Outdated Show resolved Hide resolved
src/Ethereum/Signer.cpp Outdated Show resolved Hide resolved
src/Ethereum/Transaction.h Outdated Show resolved Hide resolved
src/proto/Ethereum.proto Outdated Show resolved Hide resolved
src/proto/Ethereum.proto Outdated Show resolved Hide resolved
src/proto/Ethereum.proto Outdated Show resolved Hide resolved
src/proto/Ethereum.proto Outdated Show resolved Hide resolved
@optout21
Copy link
Contributor Author

@catenocrypt @vikmeup since this is already a breaking change, let's use string for amount/gas limit/price in proto level ?

I would not change, only because of type change.

@optout21
Copy link
Contributor Author

After some considerations, I reverted Aeternity and Aion changes, because they just create more clutter, and does not add more clarity.
Reverted commit: 7d5a03b

src/proto/Ethereum.proto Outdated Show resolved Hide resolved
src/proto/Ethereum.proto Outdated Show resolved Hide resolved
src/proto/Ethereum.proto Outdated Show resolved Hide resolved
src/proto/Ethereum.proto Outdated Show resolved Hide resolved
@hewigovens hewigovens marked this pull request as ready for review December 15, 2020 01:29
@optout21 optout21 changed the title [WIP][Breaking] Refactor Ethereum token transfer signing input [Breaking] Refactor Ethereum token transfer signing input Dec 15, 2020
@hewigovens
Copy link
Contributor

@catenocrypt @vikmeup If we merge this, we need to update TrustSDK + iOS app at the same time, and ask 3rd party apps to update SDK as well

@hewigovens hewigovens merged commit 099bb4c into master Dec 21, 2020
@hewigovens hewigovens deleted the ar/ethproto branch December 21, 2020 23:39
cornbread78 pushed a commit to cornbread78/wallet-core that referenced this pull request Dec 22, 2021
…t#1210)

* Factor out Transfer from Eth SigningInput.
* Make with oneof, Transfer and ContractGeneric.
* Remove amount from generic contract.
* Special ERC20 contract type (empty so far)
* Various factory methods for Transaction class.
* Add new ERC20 test.
* Add ERC20 handling, building.
* ABI comment
* Update iOS tests.
* Add ERC20 iOS test
* Update android tests
* Update typescript test.
* Special handling for invalid Address case.
* Additional low-level signer test.
* Small refactor in transaction building.
* Support ERC721
* ERC20 kotlin test
* Swift and Kotlin tests for ERC721
* iOS test fix
* Rename in proto
* Comment in proto, expose ERC20 call building
* Add optional payload to plain Transfer.
* ContractGeneric rename.
* Minor exception leak fix in Solana
* Payload -> Transaction rename.
* Add ERC20 Approve
* ERC20 approve iOS test
* Rename, remove transfer_ prefix
* Typescript test fix
Co-authored-by: Catenocrypt <catenocrypt@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.

[Ethereum] better SingingInput message types
5 participants