Skip to content
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

Add StateChanges in SimulateTransaction API response #963

Merged
merged 11 commits into from
May 30, 2024

Conversation

psheth9
Copy link
Contributor

@psheth9 psheth9 commented May 10, 2024

Fixes #936

This Diff has following changes:

  • Add stateDiff in SimulateTransaction API response where stateDiff interface is below
  • Add parsing of XDR types to build SimulateTxnResponse from Raw encoded XDR
  • Add unit tests

  interface LedgerEntryDiff{
    type: number;
    key: xdr.LedgerKey;
    before: xdr.LedgerEntry | null;
    after: xdr.LedgerEntry | null;
  }

Copy link

github-actions bot commented May 10, 2024

Size Change: +9.12 kB (+0.08%)

Total Size: 12 MB

Filename Size Change
dist/stellar-sdk.js 6.68 MB +5.96 kB (+0.09%)
dist/stellar-sdk.min.js 5.28 MB +3.16 kB (+0.06%)

compressed-size-action

@psheth9 psheth9 marked this pull request as ready for review May 13, 2024 16:33
@psheth9 psheth9 requested a review from Shaptic May 13, 2024 16:33
src/soroban/parsers.ts Outdated Show resolved Hide resolved
@Shaptic Shaptic mentioned this pull request May 14, 2024
4 tasks
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

Don't forget a CHANGELOG.md entry!

src/soroban/api.ts Outdated Show resolved Hide resolved
src/soroban/api.ts Outdated Show resolved Hide resolved
src/soroban/api.ts Outdated Show resolved Hide resolved
src/soroban/parsers.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/rpc/api.ts Outdated Show resolved Hide resolved
test/unit/server/soroban/simulate_transaction_test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

One last comment. Great test coverage btw! 👏

})
}),

stateChanges: sim.stateChanges?.length ?? 0 > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing it this way means stateChanges is always present, and might be set as stateChanges: undefined. We want to omit the property altogether. This is why we need the spread operator. Observe the differences:

$ node         
> {
  value: [].length > 0 ? "hello" : undefined
}
{ value: undefined }
> // vs.
> {
  ...([].length > 0 ?? { value: "hello" })
}
{}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think we are on same page for stateChanges, I thought you were proposing the same for before and after property which seemed wrong but gotchu now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was but I changed my mind - we should stick to the RPC API 👍

src/rpc/api.ts Outdated Show resolved Hide resolved
@Shaptic Shaptic merged commit e32dcd9 into master May 30, 2024
10 checks passed
@Shaptic Shaptic deleted the update-simulateTransaction-api branch May 30, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for new stateChanges in simulateTransaction
3 participants