-
Notifications
You must be signed in to change notification settings - Fork 8
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
refactor: java-crypto to follow builder/serializer improvements from latest arkecosystem/crypto changes #112
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
Complexity: Undetermined
Needs specialized, in-depth review.
Type: Refactor
The pull request improves or enhances an existing implementation.
Status: In Progress
The issue or pull request is being worked on.
labels
Feb 20, 2020
kristjank
changed the title
refactor: java-crypto
refactor: java-crypto to follow Builder/Serializer improvements from latest arkecosystem/crypto changes
Feb 20, 2020
kristjank
changed the title
refactor: java-crypto to follow Builder/Serializer improvements from latest arkecosystem/crypto changes
refactor: java-crypto to follow builder/serializer improvements from latest arkecosystem/crypto changes
Feb 20, 2020
ghost
mentioned this pull request
Feb 23, 2020
KovacZan
requested review from
faustbrian,
ItsANameToo and
kristjank
as code owners
February 25, 2020 09:56
ghost
added
Status: Needs Review
The issue or pull request needs a review by a developer of the team.
and removed
Status: In Progress
The issue or pull request is being worked on.
labels
Feb 25, 2020
kristjank
approved these changes
Mar 9, 2020
ghost
removed
the
Status: Needs Review
The issue or pull request needs a review by a developer of the team.
label
Mar 9, 2020
3 tasks
kristjank
pushed a commit
that referenced
this pull request
Mar 13, 2020
…latest arkecosystem/crypto changes (#112) (#117) * refactor: Serialized class * refactor: Deserialize class * refactor: Added abstract getTransactionInstance method to AbstractTransactionBuilder class * fix: vendorField deserialization * refactor: Transfer transaction * refactor: Abstract transaction * refactor: vote transaction * fix: TransferBuilder - added missing expiration function * refactor: SecondSignatureRegistration * refactor: ipfs transaction type * refactor: MultiPayment test * test: added IpfsBuilder test * refactor: HtlcRefund Transaction * refactor: HtlcLock Transaction * refactor: HtlcClaim Transaction * refactor: relegate resignation transaction * test: add secondVerify check to MultiPaymentBuilderTest * refactor: Delegate Registration transaction * style: apply spotless * refactor: deprecate MultiSignatureRegistration transaction * refactor: remove Fee configuration class * refactor: set default transaction amount to 0 * refactor: fees are now added in type specific builder constructors * style: apply spotlles plugin * refactor: added assetToHashMap method * style: rename assetHaspMap method to assetToHashMap * refactor: made asset classes static * test: added HtlcLock serializer tests * feat: added customAsset field * fix: remove static in classes * fix: static class * fix: maximum payment * style: run spotless apply * fix: make MultiPayment class static Co-authored-by: KovacZan <39158639+KovacZan@users.noreply.github.com>
ghost
mentioned this pull request
Mar 15, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Complexity: Undetermined
Needs specialized, in-depth review.
Type: Refactor
The pull request improves or enhances an existing implementation.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
What changes are being made?
Why are these changes necessary?
Removed configuration Fee class
Removed configuration class, because it is easier to add fee to builder by enum.
Renamed transaction builders
Renamed it because transaction type and transaction builder can't have the same name.
Refactored type specific ser/deser
Combined type Serialize and type Deserialize classes to one class, make it easier to extend and add newer transaction types.
Added new methods to AbstractTransactionBuilder class
Added
getTransactionInstance
method, which makes it possible to add new transaction types to builder.Added new abstract methods to abstract Transaction
Abstract methods were added to make it easier to extend abstract class and add new transaction types.
The following methods were added:
public abstract byte[] serialize();
public abstract void deserialize(ByteBuffer buffer);
public abstract int getTransactionType();
public abstract int getTransactionTypeGroup();
public abstract HashMap<String, Object> assetToHashMap();
Added has vendor field method to abstract Transaction
Added
hasVendorFiled
method to determent if transaction supports vendor field or not.Refactored Serializer class
Divided Serializer class in to logical methods, making code more readable.
Refactored Deserializer class
Divided Deserializer class in to logical methods, making code more readable.
Added customAsset field to TransactionAsset class
Added
customAsset
filed to make it possible to add assets for custom-transactions.Removed v1 support for transactions
Removed legacy v1 support, because it is no longer in use.
Deprecated MultiSignatureRegistration
Deprecated MultiSignatureRegistration, because it uses Schnorr signature which is not implemented in java.
How were these changes implemented?
Renamed transaction builders
Renamed transaction builders to have following syntax:
TransactionName + Builder
Refactored type specific ser/deser
Implementation for
serialize
method:Implementation for
deserialize
method:Added new method to AbstractTransactionBuilder class
Method added:
abstract Transaction getTransactionInstance();
Added new abstract methods to abstract Transaction
Added has vendor field method to abstract Transaction
Refactored Serializer class
Code divided into the following sections:
serializeCommon
serializeVendorField
serializeTypeSpecific
serializeSignatures
Refactored Deserializer class
Code divided into the following sections:
deserializeCommon
deserializeVendorField
deserializeTypeSpecific
deserializeSignature
computeId
Added customAsset field to TransactionAsset class
public HashMap<String,Object> customAsset = new HashMap<>();
Checklist