Add support for BumpFootprintExpirationOperation and RestoreFootprintOperation #491
Conversation
|
Hi @sreuland, the PR is ready, could you please review and merge it? |
src/main/java/org/stellar/sdk/BumpFootprintExpirationOperation.java
Outdated
Show resolved
Hide resolved
| .build(); | ||
| assertEquals(Integer.valueOf(123), op.getLedgersToExpire()); | ||
| assertEquals(source, op.getSourceAccount()); | ||
| String expectXdr = "AAAAAQAAAAAk4TTtavBWsGnEN3KxHw4Ohwi22ZJHWi8hlamN5pm0TgAAABkAAAAAAAAAew=="; |
There was a problem hiding this comment.
are there other tests in this repo that hardcode expected xdr, that may not be a good pattern to follow, can make the test fragile as the xdr may change, expand over time. Instead, can do a full round trip with marshaling such as secondOp = Operation.fromXdr(op.toXdrBase64()) and then assertEquals(secondOp, op)
There was a problem hiding this comment.
I tend to keep it. Our XDR module code lacks sufficient testing, and these XDR values are generated by other SDKs (Python, JS), which can serve as cross-validation. Additionally, changes in XDR definitions do not occur frequently, so even if we update it with some effort, it is acceptable.
In the previous code, there were also some validations done in this way. https://github.com/stellar/java-stellar-sdk/blob/master/src/test/java/org/stellar/sdk/OperationTest.java#L167
There was a problem hiding this comment.
yes, I have seen the usage of hardcoded xdr for serialization tests, not suggesting to change either, but that is more for scope in the xdrgen library to verify. This unit test's responsibility is to validate the code from this sdk builds a valid java object graph for BumpFootprint, and it has asserted that, it doesn't seem like the extra assertions on serialized xdr are related to code in this repo. But, I am relatively newer to this repo, and would be great if @tamirms wants to jump in with any thoughts on this also.
| @EqualsAndHashCode(callSuper = true) | ||
| @SuperBuilder(toBuilder = true) | ||
| @Value | ||
| public class BumpFootprintExpirationOperation extends Operation { |
There was a problem hiding this comment.
can you add TODO's on these that state invoke, bump, restore soroban op classes in this package still require more infrastructure before they can be used in TransactionBuilder and submission, they can't be added as an Operation in a standard 'classic' tx.
Since they have to go through preflight with rpc first, to define footprint, fees and then build out a single operation tx with all that. the js sdk, js-soroban-client, has added this functionality for tx sub of soroban ops which can be used as a reference later on for same effort here. I created new ticket to capture this effort - #495
I updated and commented on #486 to reflect the narrowing of scope for this PR to be primarily for the response object marshaling.
There was a problem hiding this comment.
I have decided not to modify it for now, see #491 (comment)
...main/java/org/stellar/sdk/responses/operations/BumpFootprintExpirationOperationResponse.java
Outdated
Show resolved
Hide resolved
close #486