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

[Ethereum] better SingingInput message types #1176

Closed
hewigovens opened this issue Nov 20, 2020 · 8 comments · Fixed by #1210
Closed

[Ethereum] better SingingInput message types #1176

hewigovens opened this issue Nov 20, 2020 · 8 comments · Fixed by #1210
Assignees
Labels
enhancement New feature or request priority:medium

Comments

@hewigovens
Copy link
Contributor

hewigovens commented Nov 20, 2020

message EthereumTransaction {
    // all default values nonce, chain id, gas and etc
    // message
    oneof contract_oneof {
        Transfer transfer = 0; (value)
        ERC20Transfer transfer_erc20 = 1; (contract, value)
        ERC20Approve approve_erc20 (contract, value)
        ERC721Transfer transfer_erc721 = 2; (contract, token_type(3 types atm), token_id)
        SmartContract smart_contract = 3; (custom data) 
    }
}
@hewigovens hewigovens added enhancement New feature or request priority:medium labels Nov 20, 2020
@vikmeup
Copy link
Contributor

vikmeup commented Nov 20, 2020

Also review other blockchains to see if they follow the same pattern.

@optout21
Copy link
Contributor

optout21 commented Dec 3, 2020

These blockchains are similar to Ethereum, could be applied to them as well:
Aeternity, Aion, Harmony, Nebulas

@optout21
Copy link
Contributor

optout21 commented Dec 3, 2020

Ethereum protobuf has an optional payload, which is used for any contract call. The caller has to put together the encoded contract call message (using methods available in wallet-core).

@vikmeup
Copy link
Contributor

vikmeup commented Dec 3, 2020

These blockchains are similar to Ethereum, could be applied to them as well:
Aeternity, Aion, Harmony, Nebulas

Great, let's apply same standards across all chains to make sure they are consistent

@optout21
Copy link
Contributor

optout21 commented Dec 8, 2020

ERC721Transfer transfer_erc721 = 2; (contract, token_type(3 types atm), token_id)

@hewigovens , I believe we use transferFrom method, with parameters from, to, and tokenId; no contract or token type. Is this OK?

function transferFrom(address _from, address _to, uint256 _tokenId) external payable;

@hewigovens
Copy link
Contributor Author

yes we only use transferFrom

@vikmeup
Copy link
Contributor

vikmeup commented Dec 9, 2020

ERC721Transfer transfer_erc721 = 2; (contract, token_type(3 types atm), token_id)

@hewigovens , I believe we use transferFrom method, with parameters from, to, and tokenId; no contract or token type. Is this OK?

function transferFrom(address _from, address _to, uint256 _tokenId) external payable;

We use both.

Here is an example from Swift code, but initially implemented by OpenSea:

All the new collectibles are using new format, but there is legacy that we need to support. It would be great to specify either the version of type of transfer call.

enum NFTVersion: String {
    case version3 = "3.0"
    case version2 = "2.0"
    case version1 = "1.0"

    init(string: String) {
        self = NFTVersion(rawValue: string) ?? .version1
    }
}

enum NFTTransfer: Equatable {
    case transfer
    case transferFrom

    static func from(version: NFTVersion) -> NFTTransfer {
        switch version {
        case .version3:
            return .transferFrom
        case .version1, .version2:
            return .transfer
        }
    }
}

@optout21
Copy link
Contributor

I checked on this, we use only method transferFrom from ERC721. In some cases ERC20.transfer is used with collectibles (with tokenID in value), but there is no transfer in ERC721.

EthereumTransactionService.swift

    static func data(for transaction: UnconfirmedTransaction, from: String) -> Data {

        case .token:
            return ERC20Encoder.encodeTransfer(to: to, tokens: transaction.value.magnitude)
        case .collectible(let collectible, _, let version):
            switch version {
            case .transferFrom:
                guard let from = AnyAddress(ethereum: from) else { return transaction.data ?? Data() }
                return ERC721Encoder.encodeTransferFrom(from: from, to: to, tokenId: collectible.tokenIDBigUInt)
            case .transfer:
                return ERC20Encoder.encodeTransfer(to: to, tokens: collectible.tokenIDBigUInt)
            }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants