Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

Properly parse simulateTransaction response variations #146

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Sep 14, 2023

This modifies the response parsing to simply attach a _parsed field onto the parsed structure, simplifying the error-prone schema detection mechanism from before.

It also updates various TypeScript definitions with the correct field types: latestLedger is a string, not a number, in most responses.

@Shaptic Shaptic requested a review from sreuland September 14, 2023 18:37
@Shaptic Shaptic self-assigned this Sep 14, 2023
@Shaptic Shaptic added the bug Something isn't working label Sep 14, 2023
)) ||
(asRaw.results ?? []).length > 0
);
return !(sim as SorobanRpc.SimulateTransactionResponse)._parsed;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@@ -235,3 +237,43 @@ function invokeSimulationResponseWithRestoration(address) {
}
};
}

describe('works with real responses', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

not suggesting a change, just wondering on hardcoding xdr as precedence in tests or build the js xdr object graph and then serialize via .toXdr() rather than hardcoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

honestly I mostly just added this as a sanity check that the specific error we were seeing got fixed. another nice part about this is that if the XDR changes, this test will fail, prompting investigation and tests with a new RPC

I agree in general that it's better to build XDR then parse it rather than hardcoding, but that also doesn't reflect real RPC behavior sometimes.. we really need to build integration tests for RPC ➡️ soroban-client, but now isn't the time for that heh

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, this isn't the place for that type of integration test, this should just work against the rpc interface, that's what system-test/e2e is for.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants