Transaction builder for transfers: EGLD / ESDTs / NFTs. Sender & receiver usernames. by andreibancioiu · Pull Request #1845 · trustwallet/wallet-core

andreibancioiu · 2021-12-15T21:30:54Z

Description

Allow transactions to specify sender's and receiver's usernames (wrt. Elrond DNS).
Allow one to create EGLD / ESDT / NFT transfers via a TransactionFactory. This one depends on a NetworkConfig object, initialized with proper (Mainnet) defaults.
Adjusted existing tests to use the well-known devnet / testnet accounts.
Breaking change: the file Elrond.proto has been redesigned to accommodate the new features, and thus the construction of SigningInput objects has to be adjusted by clients of wallet-core, as well. Before the change - Kotlin, Swift. After the change - Kotlin, Swift.

Testing instructions

Types of changes

Breaking change (fix or feature that would cause existing functionality to change) - see description.

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> ).

…elds on transaction message: sender & receiver usernames.

optout21 · 2021-12-17T07:12:38Z

@andreibancioiu , nice extensive work across all layers.
However, I have a conceptual issue with this change:
we try to keep blockchain-specific C-exposed TWXxx methods to a minimum. The preferred approach is to extend .proto messages with the transaction types, details, incl. gas details, etc.
Did you consider this approach? Would it be possible to support these transactions through the Elrond .proto file?

andreibancioiu · 2021-12-19T22:13:26Z

@catenocrypt, thanks for the input!

I will come back with the changes - define messages in the .proto file, drop the new TW* methods, and let the Signer component decide on the appropriate transaction logic (this would, in the end, delegate to the not-C-exported-anymore TransactionFactory).

hewigovens

I converted it to draft status, feel free to mark it ready to review when you make those changes

… & better tests.

hewigovens

Left some comments, overall looks good

hewigovens · 2022-01-04T04:29:49Z

+
+void normalizeHexStringTopLevel(std::string& value);
+
+std::string Codec::encodeStringTopLevel(const std::string& value) {


maybe we can remove suffix like TopLevel from all methods?

We have two distinct types of encoding (top level vs. nested) - however, indeed, the suffix was redundant in this case, I've removed it. In the future, if needed (most probably not), we might define two distinct classes: TopLevelCodec and NestedCodec.

hewigovens · 2022-01-04T04:48:38Z


-// A transaction, typical balance transfer
+// Generic / general-purpose transaction. Using one of the more specific messages (below) is recommended.
+// TODO: Ideally, we should rename this to "Transaction" or "GenericTransaction". The renaming is temporarily postponed (as of December 2021)


There are many common fields in EGLDTransfer, ESDTTransfer and ESDTNFTTransfer ( nonce/value/sender/receiver/gas/chain id), please take a look at Cosmos.proto, we can add a nested Transaction message to represent different transfer types

It's ok to do breaking changes as long as we have good test coverage (and mention it in the pr)

Indeed.

Would the structure below be all right on your side (with breaking change on SigningInput)?

Also, do you think it would be OK if we postpone this breaking change to the next PR? If not, let me know, and I will include the change in this PR.

SigningInput: bytes private_key string chain_id Gas gas oneof: - GenericTransaction - EGLDTransfer - ESDTTransfer - ESDTNFTTransfer Gas: uint64 gas_limit uint64 gas_price GenericTransaction: Accounts accounts string value string data uint32 version uint32 options EGLDTransfer: Accounts accounts string amount ESDTTransfer: Accounts accounts string amount string token_identifier ESDTNFTTransfer: Accounts accounts string amount string token_collection uint64 token_nonce Accounts (nested in transfers / generic transaction, not nested in SigningInput): uint64 sender_nonce string receiver string sender string sender_username string receiver_username

LGTM, no need to add Gas and just use gas_limit and gas_price

I've updated the Elrond.proto file and made the necessary changes - mostly in TransactionFactory.cpp and in the test files. A new class has been defined, as well: Transaction.

Thanks for the .proto refactor idea!

Perhaps it would make sense to merge the file Serialization.cpp into the new Transaction.cpp, in a future PR. Another (unrelated) idea for a future PR would be to move Transaction construction logic (currently in TransactionFactory) and gas estimation logic (currently in GasEstimator) into subclasses of the (abstract) Transaction class, such as: GenericActionTransaction, EGLDTransferTransaction, ESDTTransferTransaction etc.

hewigovens

👍，just to confirm senderUsername and receiverUsername are optional, correct?

andreibancioiu · 2022-01-07T08:14:49Z

+1，just to confirm senderUsername and receiverUsername are optional, correct?

Yes, they are optional.

optout21 · 2022-01-12T15:24:04Z

+    uint64_t forEGLDTransfer(size_t dataLength);
+    uint64_t forESDTTransfer(size_t dataLength);
+    uint64_t forESDTNFTTransfer(size_t dataLength);


Consider const for these methods.

Added const qualifier.

optout21 · 2022-01-12T15:25:23Z

+public:
+    NetworkConfig();
+
+    const std::string& getChainId();


Suggested change

const std::string& getChainId();

const std::string& getChainId() const;

Added const qualifier.

optout21 · 2022-01-12T15:26:53Z

+    // Example for ...
+    // if (timestamp > ...) {
+    //     networkConfig.setPerDataByte(...)
+    //     networkConfig.setMinGasPrice(...)
+    // }


Is this example needed?

I've removed the example.

optout21 · 2022-01-12T15:35:59Z

+    auto valueAsData = Data();
+    encode256BE(valueAsData, value, 256);
+
+    std::string encoded = normalizeHex(hex(valueAsData));


I suggest using hex(store(value, 0)), that will have no leading 0s

Applied the suggestion - I've called store() without the second & optional parameter (the second parameter is newly introduced in master and would only be available after rebase - I've skipped the rebase, which could possibly break the diff of the PR on github's code review interface).

Rigth, the parameter was just added, 0 is the default which is the same as the previous behaviour (no padding), so it should not cause issue in merge.

optout21 · 2022-01-12T15:36:42Z

+}
+
+std::string Codec::encodeUint64(uint64_t value) {
+    std::string encoded = normalizeHex(hex(value));


I suggest using hex(store(uint256_t(value), 0)), that will have no leading 0s

Applied the suggestion - I've called store() without the second & optional parameter (the second parameter is newly introduced in master and would only be available after rebase - I've skipped the rebase, which could possibly break the diff of the PR on github's code review interface).

optout21 · 2022-01-12T15:39:29Z

    return parse_hex(string.begin(), string.end());
 }

+/// "Normalizes" a hexadecimal string (redundant leading zeros are removed, even length is ensured). 


hex(Data) will always return even digits, and the same number of bytes as the input.

hex(uint64_t) will always return 8 bytes (16 hex digits), possibly with leading zeroes.

odd-length check is not needed

I prefer dropping this method, and using alternative, as suggested in the two places where it is used

Thanks for the suggestion of using store(). I've dropped this method (and its tests).

optout21 · 2022-01-12T15:42:38Z

Thanks for the rework.
I noted some missing const qualifiers on some methods, simplification of normalizeHex, otherwise all OK.

…lizeHex().

noahNewton · 2022-03-30T18:09:46Z

Signing is not working using the new strategy, even your tests are failing, fix please

hewigovens · 2022-03-31T02:02:35Z

@noahNewton Can you elaborate the issue? your sample code, error messages and expected result

noahNewton · 2022-03-31T02:35:05Z

@hewigovens

The issue is the signing is broken, I input all the required variables, receive a signature, and then when I try to POST the signature to the endpoint: https://devnet-api.elrond.com/transactions with the payload of {data: signature you gave me}, it responds with a 400 status code "Bad Request" error with the message "Sender must be provided", we have been checking very carefully and we are inputting a sender 100%, even when we take the "expectedSignature" variable from your test file (https://github.com/trustwallet/wallet-core/blob/master/android/app/src/androidTest/java/com/trustwallet/core/app/blockchains/elrond/TestElrondSigner.kt) and send this as a payload the POSt request returns the exact same error, so I think either the devnet API is broken or you're not incorporating the "sender" on your end when constructing the transaction signature

hewigovens · 2022-04-03T06:46:34Z

Will try it on our side

…iver usernames. (trustwallet#1845) * Add transaction builder for transfers: EGLD / ESDT / NFTs. Add new fields on transaction message: sender & receiver usernames. * Fix after review: less C exports, added proto structures. Refactoring & better tests. * Fix after review: remove not used files. * Fix after review (renaming, refactoring, cleanup). * Fix after review - redesign .proto structures. * Fix Android / IOS tests. * Fix after review: add missing "const" qualifiers, drop function normalizeHex().

Add transaction builder for transfers: EGLD / ESDT / NFTs. Add new fi…

6978a5b

…elds on transaction message: sender & receiver usernames.

andreibancioiu requested review from hewigovens and optout21 as code owners December 15, 2021 21:30

trustwallet deleted a comment from costeazabica11213 Dec 17, 2021

hewigovens marked this pull request as draft December 27, 2021 01:25

hewigovens reviewed Dec 27, 2021

View reviewed changes

andreibancioiu added 2 commits December 31, 2021 11:23

Fix after review: less C exports, added proto structures. Refactoring…

4974a09

… & better tests.

Fix after review: remove not used files.

424e7b0

andreibancioiu marked this pull request as ready for review December 31, 2021 12:07

andreibancioiu requested a review from hewigovens December 31, 2021 12:07

hewigovens reviewed Jan 4, 2022

View reviewed changes

Fix after review (renaming, refactoring, cleanup).

e71e5d2

andreibancioiu mentioned this pull request Jan 6, 2022

Transaction builder for transfers: EGLD / ESDTs / NFTs. Sender & receiver usernames. multiversx/wallet-core#19

Closed

5 tasks

andreibancioiu added 2 commits January 6, 2022 21:59

Fix after review - redesign .proto structures.

5a6e926

Fix Android / IOS tests.

2dfe643

andreibancioiu requested a review from hewigovens January 6, 2022 22:28

hewigovens approved these changes Jan 7, 2022

View reviewed changes

optout21 suggested changes Jan 12, 2022

View reviewed changes

Fix after review: add missing "const" qualifiers, drop function norma…

79a6c93

…lizeHex().

andreibancioiu requested a review from optout21 January 13, 2022 21:31

trustwallet deleted a comment from costeazabica11213 Jan 14, 2022

optout21 approved these changes Jan 14, 2022

View reviewed changes

optout21 merged commit 8f4fc7f into trustwallet:master Jan 14, 2022

This was referenced Jan 19, 2022

Add top Elrond assets (ESDT / SFT) - part 1 multiversx/trust-wallet-assets#2

Closed

Add top Elrond assets (ESDT / SFT) - part 1 trustwallet/assets#17512

Merged

andreibancioiu deleted the elrond-esdt branch March 21, 2022 07:52

andreibancioiu mentioned this pull request Apr 4, 2022

Add top Elrond assets (plus some cleanup) multiversx/trust-wallet-assets#3

Closed


		void normalizeHexStringTopLevel(std::string& value);

		std::string Codec::encodeStringTopLevel(const std::string& value) {

	const std::string& getChainId();
	const std::string& getChainId() const;

Conversation

andreibancioiu commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Types of changes

Checklist

Uh oh!

optout21 commented Dec 17, 2021

Uh oh!

andreibancioiu commented Dec 19, 2021

Uh oh!

hewigovens left a comment

Choose a reason for hiding this comment

Uh oh!

hewigovens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreibancioiu Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreibancioiu Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hewigovens left a comment

Choose a reason for hiding this comment

Uh oh!

andreibancioiu commented Jan 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optout21 commented Jan 12, 2022

Uh oh!

noahNewton commented Mar 30, 2022

Uh oh!

hewigovens commented Mar 31, 2022

Uh oh!

noahNewton commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hewigovens commented Apr 3, 2022

Uh oh!

andreibancioiu commented Dec 15, 2021 •

edited

Loading

andreibancioiu Jan 5, 2022 •

edited

Loading

andreibancioiu Jan 6, 2022 •

edited

Loading

noahNewton commented Mar 31, 2022 •

edited

Loading