Skip to content

feat: implement AIP 11#198

Merged
faustbrian merged 44 commits intodevelopfrom
feat/aip11
Jan 31, 2020
Merged

feat: implement AIP 11#198
faustbrian merged 44 commits intodevelopfrom
feat/aip11

Conversation

@sleepdefic1t
Copy link
Contributor

Summary

Implement AIP-11 Support.

Resolves #7 and #90.

  • completely refactor transaction classes.
  • add a test helper method to compare arrays across platforms.
  • add a standard base58 method for Ipfs hash parsing.
  • add unit test fixtures for v2 Transaction types.
  • hide MultiPayment builder tests from embedded environments.
  • use extraction to reduce map/json method complexity.
  • add more precise calculation of json payloads for ArduinoJson.

  • add and utilize packing/unpacking macros for uint value -> bytes.
  • change move -> copy/copy_n semantics to prevent unwanted modification of sources.
  • typedef -> using semantics for types .
  • BIP66 lib version 0.2.1 -> 0.3.2.
  • move BCL to an external dep.
  • update docs to reflect AIP11 changes.
  • update all relevant unit tests.

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

Additional Comments

Breaking changes:

  • Transaction Interface.
  • Transaction Builder Interface.

- bump from 2 spaces to 4 spaces
- adds support for AIP-11
- updates code style, commenting, and formatting.
- adds missing licenses in source and header files.
- updates naming conventions.
- modernizes method definitions in source files.
- avoids raw loops in favor of standard lib methods.
- improves common type-object initialization.
- improves feePolicy handling.
- improves Message class logic.
- adds a UintToString helper method.
- adds Uint byte unpacking helper methods.
- completely replaces classes within the transactions folder.
- adds a test helper method to compare arrays across platforms.
- adds a standard base58 method for Ipfs hash parsing.
- adds unit tests for new methods
- adds unit test fixtures for v2 Transaction types.
- migrates move calls to use std::move except for Uint packing.
current ESP8266 PlatformIO test builds throw errors in CI because the binary won't fit on the device.

This only affects CI, as 8266 uses separate instances for live testing.
- split transaction map and json serialization off on their own.
- use extraction to reduce map/json method complexity.
- create constants for labels and sizes.
- improve calculation of Json serialization capacity.
- rename 'getMap()' to 'addToJson()' asset methods.
- updates tests to reflect changes.
- add macros for packing `uint16`, `uint32`, and `uint64` types into a byte array.
- rename `unpack.h` -> `packing.h`.
- update headers includes in src files.
- rename `test/unpack.cpp` -> `packing.cpp`.
- update 'test/CMakeFiles.txt`.
- update unit tests.
@ghost ghost added Complexity: Undetermined Needs specialized, in-depth review. Type: Feature The issue is a request for new functionality. labels Dec 6, 2019
@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (develop@56f3317). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             develop   #198   +/-   ##
========================================
  Coverage           ?   100%           
========================================
  Files              ?     61           
  Lines              ?   1181           
  Branches           ?      0           
========================================
  Hits               ?   1181           
  Misses             ?      0           
  Partials           ?      0
Impacted Files Coverage Δ
src/transactions/types/htlc_refund.hpp 100% <ø> (ø)
src/transactions/types/multi_payment.hpp 100% <ø> (ø)
src/transactions/types/multi_payment.cpp 100% <ø> (ø)
src/transactions/types/htlc_lock.cpp 100% <ø> (ø)
src/transactions/types/second_signature.cpp 100% <ø> (ø)
src/transactions/types/delegate_registration.cpp 100% <ø> (ø)
src/transactions/defaults/fees.hpp 100% <ø> (ø)
src/transactions/types/htlc_claim.hpp 100% <ø> (ø)
src/transactions/types/ipfs.cpp 100% <ø> (ø)
src/transactions/types/vote.cpp 100% <ø> (ø)
... and 29 more

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 56f3317...5cad795. Read the comment docs.

@sleepdefic1t
Copy link
Contributor Author

@vasild
On the previous discussion..

I agree, if it can’t serde, it’s not a good candidate for support.

It’s not so much a matter of whether a platform can support serde though;

I’m more concerned with the space unit tests and fixtures take up.

The library itself doesn't take up much space and is fairly well-oriented towards embedded.

I can sign a Tx on 8-bit platforms (Uno); it’s slow, but works.

Adding multiple fixtures exceeds the available space though.

I’d like to eventually find a way of optimizing such that storage is improved, but testability isn’t affected.

@ciband has helped improve this situation by breaking up tests so that different aspects can be tested independently. I think this should be good enough in the mean time.



I’m leaving this out of the AIP11 feature as it’s not directly concerned with the scope here, but will continue looking at the issue separately 👍

@sleepdefic1t sleepdefic1t requested a review from vasild December 9, 2019 20:45
Copy link
Contributor

@ciband ciband left a comment

Choose a reason for hiding this comment

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

I just did some spot checking and made some comments about general patterns and practices.

- `...(const uintX_t)` -> `(uint8_t)`.
- `...(const uintX_t&)` -> `(uint8_t)`.
Detect and improve performance and memory safety using Sanitization.

Done using the following flags:
- `ASAN_OPTIONS=detect_leaks=1 `
- `-fsanitize=address,undefined,float-divide-by-zero,unsigned-integer-overflow`
- `-fno-sanitize-recover=all`
- `-fno-optimize-sibling-calls`
- `-fsanitize-address-use-after-scope`
- `-fno-omit-frame-pointer`
- `-g`
- `-O1`

Changes:
- improve signature memory initialization and handling.
- `std::move` -> `std::copy` and `std::copy_n` to avoid misusing non-owned values.
- change some types to avoid bad comparison and invalid shifts.
- refactor Hex Tools to avoid undefined behavior.
- refactor some test cases to avoid dangling ptrs.
- use `constexpr` for global vars where possible.
- `static const` -> `const` for non-literal globals.
- update docs and examples to reflect these changes.
@sleepdefic1t sleepdefic1t added Complexity: High More than 256 lines changed. Status: Needs Review The issue or pull request needs a review by a developer of the team. and removed Complexity: Undetermined Needs specialized, in-depth review. labels Jan 9, 2020
- add `id` member to `TransactionData`.
- add calls to set txId from `sign`, `serialize`, and `deserialize`.
- move txId mapping call to Mapping class.
- add tests to maintain coverage.
- add additional security by zeroing created keypairs during the sign/2nd sign logic.
- split mapping into separate mapping and json files.
- move json and mapping to a new mapping directory.
- create a separate file for shared mapping labels.
- rename json and mapping intern static methods to make intent more clear.
- rename `Mapping::getMap` -> `Mapping::fromTransactionData`.
- rename `Mapping::toJson` -> `Json::fromTransactionMap`.
- cleanup json and mapping comments.
- change json logic to display txId before signatures.
- update unit tests to reflect any changes.
Several transaction-type models use the longer-form `std::array` pattern.

A few predefined array-types are also already available in `interfaces/identities` to improve readability and intent.

The following were updated within the transaction asset classes:

- `std::array<uint8_t, HASH_32_LEN>` -> `Hash32`
- `std::array<uint8_t, ADDRESS_HASH_LEN>` -> `AddressHash`

The asset types being updated:

* `htlc_lock`
* `htlc_claim`
* `htlc_refund`
* `transfer`

These changes are non-breaking.
@faustbrian faustbrian merged commit 09714c5 into develop Jan 31, 2020
@ghost ghost deleted the feat/aip11 branch January 31, 2020 04:39
@ghost ghost removed the Status: Needs Review The issue or pull request needs a review by a developer of the team. label Jan 31, 2020
This was referenced Jan 31, 2020
@sleepdefic1t sleepdefic1t mentioned this pull request Feb 13, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complexity: High More than 256 lines changed. Type: Feature The issue is a request for new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants