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

Rosetta Implementation - pt4 (Stage 3.4 of Node API Overhaul) #3380

Merged
merged 34 commits into from
Oct 9, 2020

Conversation

Daniel-VDM
Copy link
Contributor

@Daniel-VDM Daniel-VDM commented Oct 6, 2020

Stage 3.4 of Node API Overhaul

TLDR: This PR implements Rosetta's Construction API as well as correct some small bugs in the Node API found during the integration test of the Construction API. The integration test was done using the rosetta-cli using this configuration. Lastly, some refactoring of the block transaction formatter was done to make it more general so that the Construction API could consume said formatter.

Constriction API Details

For reference, this is the flow of the Construction API:

                               Caller (i.e. Coinbase)                + Construction API Implementation
                              +---------------------------------------------------------------------------------+
                                                                     |
                               Derive Address   +----------------------------> /construction/derive
                               from Public Key                       |
                                                                     |
                             X                                       |
                             X Create Metadata Request +---------------------> /construction/preprocess
                             X (array of operations)                 |                    +
    Get metadata needed      X                                       |                    |
    to construct transaction X            +-----------------------------------------------+
                             X            v                          |
                             X Fetch Online Metadata +-----------------------> /construction/metadata (online)
                             X                                       
                                                                     |
                             X                                       |
                             X Construct Payloads to Sign +------------------> /construction/payloads
                             X (array of operations)                 |                   +
                             X                                       |                   |
 Create unsigned transaction X          +------------------------------------------------+
                             X          v                            |
                             X Parse Unsigned Transaction +------------------> /construction/parse
                             X to Confirm Correctness                |
                             X                                       |
                                                                     |
                             X                                       |
                             X Sign Payload(s) +-----------------------------> /construction/combine
                             X (using caller's own detached signer)  |                 +
                             X                                       |                 |
   Create signed transaction X         +-----------------------------------------------+
                             X         v                             |
                             X Parse Signed Transaction +--------------------> /construction/parse
                             X to Confirm Correctness                |
                             X                                       |
                                                                     |
                             X                                       |
                             X Get hash of signed transaction +--------------> /construction/hash
Broadcast Signed Transaction X to monitor status                     |
                             X                                       |
                             X Submit Transaction +--------------------------> /construction/submit (online)
                             X                                       |
                                                                     +

All endpoints do NOT have access to the current state of the chain/pool unless specified that it is 'online'.

/construction/derive | Implementation

An Address Identifier can be derived from a compressed secp256k1 public key. The key's bytes given to this endpoint are assumed to be said compressed key.

/construction/preprocess | Implementation

One can determine if a set of operations creates a valid Harmony Transaction by using this endpoint. The underlying function(s) to check the operation(s) can be found here. Once the operations are validated, all metadata options (and curred data) are packaged into this struct and returned to the client to be forwarded (unchanged) on to the /construction/metadata endpoint.

/construction/metadata | Implementation

This endpoint is considered to be 'online'. Given the metadata option struct from /construction/preprocess, it fetches the correct nonce & gas fee estimation and returns it in this struct to the client to be forwarded (unchanged) on to the /construction/payloads endpoint.

/construction/payloads | Implementation

This endpoint actually constructs the Harmony transaction given the metadata from /construction/metadata and OperationComponents from the given operations. The returned payload to be signed by the client is the 'signer' (EIP115 Signer) hash of a Harmony transaction (i.e: this). Note that the returned harmony transaction object is wrapped in this struct.

/construction/parse | Implementation

One can check that an unsigned or signed (wrapped) transaction results in the intended rosetta operations using this endpoint. Note that parsing uses the same formatting functions as in the /block endpoint of the Node API. Logic for unpacking and checking the wrapped transaction can be found here.

/construction/combine | Implementation

One can get a signed (wrapped) transaction object by giving the signed payload to this endpoint (along with the unsigned wrapped transaction).

/construction/hash | Implementation

One can get the hash of a signed (wrapped) transaction using this endpoint.

/construction/submit | Implementation

This endpoint is considered to be 'online'. One can submit a signed (wrapped) transaction to the pool using this endpoint.

Misc Details

The transaction formatting needed to be changed to not error on unsigned transactions. Hence the introduction of the DefaultSenderAddress.

Moreover, all error messages now also return the line number, file, and node version to help with debugging.

Testing

Lots of effort was put into writing unit tests. Moreover, test scenarios were written for the rosetta-cli to test the construction API. One can run these scenarios by downloading and installing the rosetta-cli, and running the tests with the following command once localnet is started:

./rosetta-cli check:construction --configuration-file ./examples/configuration/harmony.json

The scenario file is here

Here is a screenshot of the successful tests:

image

Note that this also checks a large part of the Node API as it uses it to ensure that transactions are finalized.

TODO

Once merged, all functionalities of the constriction endpoint will be tested on testnet.

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add network ID check to all construct endpoints

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add UnsupportedCurveTypeError

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Implement ConstructionDerive & add ConstructAPI router

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Make recover middleware the outermost middleware

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Correct error for block not found

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Add GetValidConstructionOperations

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Make TransactionMetadata optional ptr

* Add UnmarshalFromInterface for TransactionMetadata

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add InvalidTransactionConstructionError to network response

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add ConstructionPreprocess & ConstructionMetadata skeleton

* Add Helper methods for said functions

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add GasPrice to ConstructMetadata

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Make suggested gas fee & price its own fn

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Nit - fix comment

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add To & From Account ID in tx metadata

* remove getAmountFromUndelegateMessage
* make contract address a valid account identifier
* remove needless metadata in non-staking operations

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Use sdk MapMarshall instead of rpc structured response

* remove unused GetValidConstructionOperations
* Add CurrencyHash

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* refactor usages of rpc structured response for sdk map marshall
* add account identifier checks for transaction metadata

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Max related operation check more general for tx op check

* refactor names for readability

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Remove txAccounts & add OperationComponents to metadata for quicker processing
* Remove From & To acc ID from TransactionMetadata as it's in OperationComponents
* Update tests for changes

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add tx type to OperationComponents

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add logs to TransactionMetadata

* Enforce Operation type uniqueness invariant for each transaction

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add staking operation medata type declaration

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Fix error messages & update comment

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Implement getCrossShardOperationComponents

* rename txAccs to components for clarity

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Implement getContractCreationOperationComponents

* Fix some edge case optional checks for operation validation

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Update broken rosetta structs

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add ChainID to ConstructMetadata

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Refactor formatTransaction for clarity

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Move maxNumOfConstructionOps to operation_components.go

* Clarify unsupported type err msg

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Implement framework for ConstructionPayloads

* Remove currying OperationComponents in metadata as PublicKeys is now
part of the request in the necessary endpoints
* Add respective checks for added PublicKeys
* Make getAddressFromPublicKey more general & update tests

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add unmarshal check for ConstructMetadata & ConstructMetadataOptions

* Remove constructTransaction
* Wrap unsigned transaction with UnsignedTransaction to include intended signer
* Add framework for ConstructionParse

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add getSigningPayload method to construction

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Add unpackWrappedTransactionFromHexString & refactor rlp encoding of transactions
* Make getSigningPayload return a list

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add TestUnpackWrappedTransactionFromHexString

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Move unpackWrappedTransactionFromHexString to ConstructionParse
before parsing transactions bases on if it's signed or not

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Nit - fix WrappedTransaction comments

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add extra negative tests for unpackWrappedTransactionFromHexString

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Implement ConstructionCombine
* Implement ConstructionHash
* Implement ConstructionSubmit
* Ensure RLPbytes & From is present in WrappedTransaction
when unpackWrappedTransactionFromHexString is called

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Refactor construction into separate files

* Move tests into appropriate files

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Update edge case for getSuggestedFeeAndPrice found during testing

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Clarify names for construction_create_test.go

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Fix ConstructionParse to not use crypto.PubkeyToAddress

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Fix UnmarshalFromInterface for CrossShardTransactionOperationMetadata

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Implement TestGetContractCreationOperationComponents
* Implement TestGetCrossShardOperationComponents

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Finish operation_components_test.go

* Implement TestGetTransferOperationComponents
* Implement TestGetOperationComponents

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Fix big number checks for getTransferOperationComponents

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Implement TestConstructPlainTransaction
* Implement TestConstructCrossShardTransaction
* Implement TestConstructContractCreationTransaction

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add shard ID checks for constructCrossShardTransaction

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add bad curve test for TestGetAddressFromPublicKey

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add nil error catches for Construction API

* Fix nits in messages as seen during pass
* Add DefaultGasLimit

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add trace message with version to all err msgs

* Add NewError unit test

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Edge case bug fixes

* Create empty TransactionMetdata if no metadata is provided in operations.
Also update tests to correctly account for the behavior.
* In pre-staking era make current block one less than the absolute latest
to guarentee calculation of pres-taking eara block rewards.

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Rename function to unpackWrappedTransactionFromString
* Update respective tests

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Make parsed transaction statuses empty

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Remove needles list initialization

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Fix genesis network status crash

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add OperationType to ConstructMetadataOptions

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Fix rosetta service rebase

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add formatNegativeValue to ensure there's only 1 zero

* Correct regression found in constructCrossShardTransaction

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>

[rosetta] Add contract creation estimate gas hack

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
* Update Construction API parser to reflect change

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
@Daniel-VDM Daniel-VDM added the rpc RPC or API label Oct 6, 2020
@Daniel-VDM Daniel-VDM requested review from rlan35 and LeoHChen October 6, 2020 07:16
@Daniel-VDM Daniel-VDM self-assigned this Oct 6, 2020
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Copy link
Contributor

@LeoHChen LeoHChen left a comment

Choose a reason for hiding this comment

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

did a quick review, in general looks good to me with lots of good unit tests.

rosetta/common/errors.go Outdated Show resolved Hide resolved
rosetta/common/errors.go Outdated Show resolved Hide resolved
rosetta/services/block.go Show resolved Hide resolved
rosetta/services/block.go Show resolved Hide resolved
rosetta/services/construction.go Show resolved Hide resolved
rosetta/services/construction_create.go Outdated Show resolved Hide resolved
rosetta/services/network.go Show resolved Hide resolved
rosetta/services/operation_components.go Outdated Show resolved Hide resolved
rosetta/services/operation_components.go Show resolved Hide resolved
rosetta/services/transaction_construction.go Show resolved Hide resolved
* Add GetCallStackInfo to internal utils
* Add EstimateGas TODO in RPC package
* Remove DefaultGasLimit to use param gas limit

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
"message": err.Error(),
})
}
if amount.Sign() == 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

for contract creation, you could still send token along to the contract address right? By making the contract constructor payable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, but then the amount should be negative for the rosetta spec as it would deduct funds from the contract deployer.

@rlan35
Copy link
Contributor

rlan35 commented Oct 7, 2020

any risk that these new codes will cause issues for mainnet validators? Seems very separated.

@Daniel-VDM
Copy link
Contributor Author

any risk that these new codes will cause issues for mainnet validators? Seems very separated.

Rosetta is only enabled by default on the explorer node. Validators have to manually enable it, therefore there shouldn't be any issues.

* Remove needless block check for genesisBlock

Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Signed-off-by: Daniel Van Der Maden <dvandermaden0@berkeley.edu>
Copy link
Contributor

@LeoHChen LeoHChen left a comment

Choose a reason for hiding this comment

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

LGTM in general, thanks for the work.

@Daniel-VDM Daniel-VDM merged commit 2844d73 into harmony-one:main Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rpc RPC or API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants