Code updates for latest changes#3
Conversation
| // Alice transfers to Bob | ||
| String bobCustomData = "Bob's custom data"; | ||
| byte[] bobStateData = bobCustomData.getBytes(StandardCharsets.UTF_8); | ||
| DataHash bobDataHash = new DataHasher(HashAlgorithm.SHA256).update(bobStateData).digest(); |
There was a problem hiding this comment.
how does Alice know Bob's custom data or its hash?
|
|
||
| public <T extends MintTransactionData<?>> CompletableFuture<SubmitCommitmentResponse> submitCommitment( | ||
| MintCommitment<T> commitment) { | ||
| return this.client.submitCommitment( |
There was a problem hiding this comment.
aggregator client coult simply take Commitment object as input, MintCommitment and TransferCommitment both extend it. TransactionData could know how to calculate its own hash (MintTransactionData already does) and TransferTransactionData has all the token info it needs at creation time.
| return token.update(state, transaction, nametagTokens); | ||
| } | ||
|
|
||
| public CompletableFuture<InclusionProofVerificationStatus> getTokenStatus( |
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Direct address implementation |
| } No newline at end of file | ||
| public MaskedPredicate( | ||
| byte[] publicKey, | ||
| String algorithm, |
|
|
||
| // Extract algorithm from first two bytes | ||
| int algorithmValue = ((imprint[0] & 0xFF) << 8) | (imprint[1] & 0xFF); | ||
| HashAlgorithm algorithm = HashAlgorithm.values()[algorithmValue]; |
There was a problem hiding this comment.
potential ArrayIndexOutOfBoundsException
| return false; | ||
| } | ||
| DirectAddress that = (DirectAddress) o; | ||
| return Objects.equals(data, that.data) && Objects.deepEquals(checksum, |
There was a problem hiding this comment.
why not Arrays.equals instead of deepEquals
| return false; | ||
| } | ||
| ProxyAddress that = (ProxyAddress) o; | ||
| return Objects.equals(data, that.data) && Objects.deepEquals(checksum, |
There was a problem hiding this comment.
Pull Request Overview
This is a comprehensive refactoring PR that updates the codebase to align with the latest changes across multiple components. It includes significant restructuring of the core SDK classes and removal of outdated test files.
Key changes:
- Complete refactoring of hash, transaction, token, and predicate classes to remove dependencies on async patterns and outdated interfaces
- Removal of numerous test files that appear to be outdated or duplicated
- Addition of new core utilities like
BitString,VerificationResult, and split transaction functionality - Migration from
ISerializableinterface to more specific transaction data interfaces
Reviewed Changes
Copilot reviewed 272 out of 283 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Test files (removed) | Cleanup of outdated test infrastructure including JSONRPC, CBOR, and hash-related tests |
| New test files | Addition of comprehensive tests for core functionality including Merkle trees, API components, and E2E flows |
| Core SDK refactoring | Complete overhaul of transaction, token, and utility classes with new verification patterns |
| New utilities | Introduction of BitString, VerificationResult, and token splitting functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| throw new Exception(String.format("Failed to submit nametag transfer commitment: %s", | ||
| nametagMintResponse.getStatus())); |
There was a problem hiding this comment.
The error message references nametagMintResponse.getStatus() but should reference nametagSecondUseResponse.getStatus() since this is handling the failure of the second use nametag transfer, not the mint response.
| throw new Exception(String.format("Failed to submit nametag transfer commitment: %s", | |
| nametagMintResponse.getStatus())); | |
| nametagSecondUseResponse.getStatus())); |
| if (status == InclusionProofVerificationStatus.PATH_NOT_INCLUDED) { | ||
| CompletableFuture.delayedExecutor(intervalMillis, TimeUnit.MILLISECONDS) | ||
| .execute(() -> checkInclusionProof(client, commitment, future, startTime, timeoutMillis, | ||
| intervalMillis)); | ||
| } else { | ||
| future.completeExceptionally( | ||
| new RuntimeException(String.format("Inclusion proof verification failed: %s", status))); | ||
| } |
There was a problem hiding this comment.
Missing return statement after scheduling retry in the PATH_NOT_INCLUDED case. This will cause both the retry to be scheduled AND the exception to be thrown, leading to unexpected behavior.
|
|
||
| this.id = id; | ||
| this.type = type; | ||
| this.data = data == null ? null : Arrays.copyOf(data, data.length); |
There was a problem hiding this comment.
Potential NullPointerException when data is null. The condition checks if data is null but then calls Arrays.copyOf(data, data.length) which will throw NPE if data is null. Should be: this.data = data != null ? Arrays.copyOf(data, data.length) : null;
| this.data = data == null ? null : Arrays.copyOf(data, data.length); | |
| this.data = data != null ? Arrays.copyOf(data, data.length) : null; |
| return aggregatorClient; | ||
| } | ||
| return this.client.submitCommitment(commitment.getRequestId(), commitment.getTransactionData() | ||
| .calculateHash(token.getId(), token.getType()), commitment.getAuthenticator()); |
There was a problem hiding this comment.
to extend my previous comment: transfer could also have commitment.getTransactionData().calculateHash() with no arguments and inside calculateHash() there might be a lambda which has token id and type in its closure
|
|
||
| String[] result = address.split("://", 2); | ||
| if (result.length != 2) { | ||
| throw new IllegalArgumentException("Invalid address format"); |
There was a problem hiding this comment.
include invalid input in the message, easier to read logs and figure out what went wrong
|
|
||
| this.id = id; | ||
| this.type = type; | ||
| this.data = data == null ? null : Arrays.copyOf(data, data.length); |
| byte[] imprint = new byte[this.data.length + 2]; | ||
| int algorithmValue = this.algorithm.getValue(); | ||
| imprint[0] = (byte) ((algorithmValue & 0xFF00) >> 8); | ||
| imprint[1] = (byte) (algorithmValue & 0xFF); |
There was a problem hiding this comment.
could be moved to HashAlgorithm as e.g. getImprintPrefix
No description provided.