Add the fromXdrBase64, fromXdrByteArray, toXdrBase64 and toXdrByteArray methods to the XDR classes. #503
Conversation
overcat
changed the title
|
This PR is ready for review and merge. |
|
@overcat , Can you provide a brief overview or drill down on where to focus for real logic/functionality changes, key files to look at? there's 367 changed files in the pr and trying to find efficient way to understand what's been done vs. compile-time fixes. [edit] - I can ignore the entire The one follow-up is can you add a comment on the readme.md in the xdrgen section that states what git ref of stellar/xdrgen and stellar/stellar-xdr/next was used? |
| try { | ||
| return ClaimableBalanceID.decode(balanceIdXdrDataInputStream); | ||
| return ClaimableBalanceID.fromXdrByteArray(balanceIdBytes); |
There was a problem hiding this comment.
this seems too specific to claimablebalance to be in a generic util, and since you've added the xdr helper methods, how about refactoring this and deleting these claimable balance methods from Util and clients can just inline the one remaining line in the method for same result?
| LedgerEntry.LedgerEntryData ledgerEntryData = | ||
| ledgerEntryDataFromXdrBase64(entries.get(0).getXdr()); | ||
| LedgerEntry.LedgerEntryData ledgerEntryData; | ||
| try { |
There was a problem hiding this comment.
| try { | |
| try { | |
| LedgerEntry.LedgerEntryData ledgerEntryData = LedgerEntry.LedgerEntryData.fromXdrBase64(entries.get(0).getXdr()); | |
| long sequence = ledgerEntryData.getAccount().getSeqNum().getSequenceNumber().getInt64(); | |
| return new Account(accountId, sequence); | |
| } catch (IOException e) { | |
| throw new IllegalArgumentException("Invalid ledgerEntryData: " + entries.get(0).getXdr(), e); | |
| } |
rather than declare ledgerEntryData outside of try/catch, just use it inside when it's valid.
There was a problem hiding this comment.
I personally prefer to include as little code as possible within the try block. Assuming there is some other business logic before returning Account, should we also place it within the try block?
| @@ -421,7 +427,11 @@ private Transaction assembleTransaction( | |||
| List<SorobanAuthorizationEntry> newEntries = new ArrayList<>(originalEntries); | |||
| if (simulateHostFunctionResult.getAuth() != null) { | |||
| for (String auth : simulateHostFunctionResult.getAuth()) { | |||
| newEntries.add(sorobanAuthorizationEntryFromXdrBase64(auth)); | |||
| try { | |||
| newEntries.add(SorobanAuthorizationEntry.fromXdrBase64(auth)); | |||
There was a problem hiding this comment.
@overcat , please refer to the js soroban client for a reference on how it merges simulation results to original transaction,
https://github.com/stellar/js-soroban-client/blob/main/src/transaction.ts#L21
the java sdk should be consistent to how that handles merging auths, the js sdk will not apply simulated auths if the original tx had auths.
There was a problem hiding this comment.
overall looks good, just need to get response on the simulation results with auth, left comment - https://github.com/stellar/java-stellar-sdk/pull/503/files#r1289105610
can merge after that is resolved, thanks!
overcat
force-pushed
the
base64-methods
branch
from
abb709b
to
fb44420
Compare
Let's track it in #502 and I'm pushing it forward. |
…DRNEXT_COMMIT=e372df9f677961aac04c5a4cc80a3667f310b29f
Closes #501
The XDR class generation in this PR depends on stellar/xdrgen#164
Breaking changes
Utils.claimableBalanceIdToXDRandUtils.xdrToClaimableBalanceId.