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

Bug: json.Unmarshal cannot directly parse Transaction interface in SimulateTransactionInput #608

Open
haisenCx opened this issue Jul 29, 2024 · 10 comments

Comments

@haisenCx
Copy link

Description

I encountered a bug while running unit tests using starknetGo. The issue occurs when the code
"err = simulateTxIn.UnmarshalJSON(simulateTxnRaw)" attempts to parse the ./tests/trace/simulateInvokeTx.json file.
The error message is the img below:
微信图片_20240729153932
I think the error message indicates that json.Unmarshal cannot directly parse the Transaction interface in the SimulateTransactionInput struct

Steps to Reproduce

Run the test TestSimulateTransaction

Suggested Fix

To fix this issue, I suggest implementing a custom UnmarshalJSON method for the SimulateTransactionInput struct. This method will handle the parsing of the Transaction interface by determining the specific struct type.

@thiagodeev
Copy link
Collaborator

Hi @haisenCx, sorry for the delay. I ran the test here and everything went well. Could you try again?
I see that the error occurs in line 122, but this line doesn't contain any "relevant" code logic. Have you edited the code?

@haisenCx
Copy link
Author

Hi @haisenCx, sorry for the delay. I ran the test here and everything went well. Could you try again? I see that the error occurs in line 122, but this line doesn't contain any "relevant" code logic. Have you edited the code?
1723794652876

Hello, @thiagodeev .Yes, I had modified some code before, so the line numbers in my previous screenshot do not match. Please take a look at this new screenshot I provided above. This is the code from the starknetgo project that I re-cloned, and it is still throwing errors.

@haisenCx
Copy link
Author

The issue occurs when running tests in the mainnet env.

@thiagodeev
Copy link
Collaborator

Sorry @haisenCx, I forgot this test was supposed to run on mainnet 😅.
Since you made a valid suggestion, could you try to adjust it?

@haisenCx
Copy link
Author

Sorry @haisenCx, I forgot this test was supposed to run on mainnet 😅. Since you made a valid suggestion, could you try to adjust it?

Yes,My pleasure🤓

@thiagodeev
Copy link
Collaborator

Thanks! 😁
A suggestion I can give is to focus on the Transaction type itself, not the entire SimulateTransactionInput.
I took a look and noticed that the spec says the input value should be a BroadcastTx, and this is not happening. I believe we could make something like that to have a generic BroadcastTx type and then make a custom Unmarshal function to it. This could solve this bug.

Feel free to think of other ideas, bonus for simplicity

@haisenCx
Copy link
Author

Thanks! 😁 A suggestion I can give is to focus on the Transaction type itself, not the entire SimulateTransactionInput. I took a look and noticed that the spec says the input value should be a BroadcastTx, and this is not happening. I believe we could make something like that to have a generic BroadcastTx type and then make a custom Unmarshal function to it. This could solve this bug.

Feel free to think of other ideas, bonus for simplicity

Okay, I think your suggestion is the right way, we should follow the spec. I'll take another look, and then I'll post my solution here later.

Copy link

onlydustapp bot commented Aug 16, 2024

Hey @haisenCx!
Thanks for showing interest.
We've created an application for you to contribute to Starknet.go.
Go check it out on OnlyDust!

@haisenCx
Copy link
Author

Hello @thiagodeev, do you mean to change the Transaction in SimulateTransactionInput struct to the BroadcastTxn structure? I see that a BroadcastTxn struct has already been defined in the project. Can it be used directly?

@haisenCx
Copy link
Author

QQ截图20240817115234
I tried using the BroadcastTxn that has already been defined in the project directly, and it solved the UnmarshalJSON issue. However, a new problem has arisen: the expected result and the actual result are not equal.

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

No branches or pull requests

2 participants