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

feat: support new APIs from StarkNet v0.8.0 #90

Merged
merged 5 commits into from Mar 23, 2022
Merged

Conversation

xJonathanLEI
Copy link
Owner

Resolves task 2 of #85:

add support for new gateway APIs

This PR uses a special deserializer for the amount field in FeeEstimate because for some unknown reasons it just won't work out of the box.

@milancermak
Copy link
Collaborator

I just ran a simple test to see the issue with the amount deser - I removed the #[serde(deserialize_with = "deserialize")] in FeeEstimate.rs, but it all worked like a charm. What was the error you were getting? Can you try and reproduce again?

@xJonathanLEI
Copy link
Owner Author

Hmmm lemme try to make it into a test case ser.

@milancermak
Copy link
Collaborator

Went through the rest, all looks good.

@xJonathanLEI
Copy link
Owner Author

There you have it, the new test case test_estimate_fee_deser fails if you remove the #[serde(deserialize_with = "deserialize")] line.

Note that the target type is GatewayResponse<FeeEstimate> instead of FeeEstimate. Deserializing directly to FeeEstimate succeeds with or without the attribute. That's why I have to put the test case in starknet-providers instead of starknet-core.

This is a very similar situation with the arbitrary_precision issue we had before, so I think they're related. For context, this is what's causing the issue last time:

serde-rs/serde#1183

But it might not be related after all, since that issue was rather on format-specific stuff like Number, which we're not using here.

@milancermak
Copy link
Collaborator

I'm confused now. What's the point of the test if the call to estimate_fee succeeds even without the #[serde(deserialize_with = "deserialize")] field attribute? Why the wrapping with GatewayResponse?

I tried reproducing with this code (which is the same call as in the test case) and it worked either way:

use starknet::{
    core::types::{BlockId, FieldElement, InvokeFunctionTransactionRequest},
    providers::{Provider, SequencerGatewayProvider},
};

#[tokio::main]
async fn main() {
    let provider = SequencerGatewayProvider::starknet_alpha_goerli();
    let tx = InvokeFunctionTransactionRequest {
        contract_address: FieldElement::from_hex_be(
            "0x07394cbe418daa16e42b87ba67372d4ab4a5df0b05c6e554d158458ce245bc10",
        )
        .unwrap(),
        entry_point_selector: FieldElement::from_hex_be(
            "0x02f0b3c5710379609eb5495f1ecd348cb28167711b73609fe565a72734550354",
        )
        .unwrap(),
        calldata: vec![
            FieldElement::from_dec_str(
                "2536338608804621486891098924999890751656158566880912297504415061810375427475",
            )
            .unwrap(),
            FieldElement::from_dec_str("1000000000000000000000").unwrap(),
            FieldElement::ZERO,
        ],
        signature: vec![],
    };
    let fee = provider.estimate_fee(tx, BlockId::Latest).await.unwrap();

    dbg!(fee);
}

@xJonathanLEI
Copy link
Owner Author

The wrapping with GatewayResponse is necessary because not all fee estimation requests will succeed. You can't capture the error responses if you don't wrap it inside GatewayResponse.

For the second part, I tried running your code and yes it works with or without the attribute, which is quite weird, because you're still deserializing into GatewayResponse, but it works here, and it shouldn't.

Lemme look deeper into why this is working at all...

@xJonathanLEI
Copy link
Owner Author

So I made another test project that's really just the same as the failing test case:

use starknet::{core::types::FeeEstimate, providers::GatewayResponse};

fn main() {
    serde_json::from_str::<GatewayResponse<FeeEstimate>>(
        "{\"amount\": 1630000000000, \"unit\": \"wei\"}",
    )
    .unwrap();
}

and it works. Very strange. It looks like it's only failing within our library.

@xJonathanLEI
Copy link
Owner Author

Does it mean it only fails under #[cfg(test)]?

@xJonathanLEI
Copy link
Owner Author

OK I figured it out. Turns out it's a bug that's fixed in serde_json v1.0.75...

The standalone test project works because it's not locked to 1.0.74 like our project.

@xJonathanLEI
Copy link
Owner Author

Specifically, it's fixed here:

serde-rs/json#848

@xJonathanLEI
Copy link
Owner Author

OK lemme upgrade serde_json and remove the attribute lol

@milancermak
Copy link
Collaborator

Well done on figuring it out 💪

Copy link
Collaborator

@milancermak milancermak left a comment

Choose a reason for hiding this comment

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

@xJonathanLEI xJonathanLEI merged commit b4e48ec into master Mar 23, 2022
@xJonathanLEI xJonathanLEI deleted the dev/0_8_0_apis branch March 23, 2022 14:15
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.

None yet

2 participants