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(contracts): add tx_id to FuelCallResponse #734

Merged
merged 10 commits into from
May 22, 2023

Conversation

Br1ght0ne
Copy link
Contributor

@Br1ght0ne Br1ght0ne commented Dec 8, 2022

Description of changes

Close #733.

  • for scripts, tx_id is Transaction::Script.id()
  • for contract calls, tx_id is ExecutableFuelCall.tx.id()

Checklist

  • I have linked to any relevant issues.
  • I have updated the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary labels.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Short and neat. gg!

Just gotta fix the tests, but after that feel free to merge!

Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Changing my review until the test gets fixed to prevent an accidental merge haha.

@digorithm digorithm requested a review from a team December 8, 2022 03:56
@Br1ght0ne Br1ght0ne requested a review from digorithm December 8, 2022 04:19
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

Just one question regarding async.

packages/fuels-contract/src/contract.rs Outdated Show resolved Hide resolved
@Br1ght0ne Br1ght0ne force-pushed the oleksii/call-response-txid branch from ddf32b3 to dd13efe Compare January 16, 2023 14:58
@Br1ght0ne Br1ght0ne requested review from iqdecay and MujkicA January 16, 2023 16:00
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

So I like that we don't need to call the network to get the tx_id, but I do agree with the comment about changing the get_response API.
First of all this is afaiu a breaking change that we should consider carefully.
Second, I agree with you that it is wonky.
I'd like to see what a caching solution looks like 😄

Can't we include an extra Receipts type that just gives us the txid? But it would feel bad to "lie" about it being a tx receipt.

EDIT: @digorithm can you weigh in on priority vs (having an async || changing the API)?

@digorithm
Copy link
Member

Hmm, seems like we're bound to modify the API, regardless of the options. Having to append .await is a change to its API, anyway.

Are we deciding between let response = call_handler.get_response(receipts).await?; and let response = call_handler.get_response(receipts, executable_call.tx.id())?;?

If so, the first option doesn't require the user to know more internal details and manually fetch the tx id, which I believe to be better than the second option.

@iqdecay
Copy link
Contributor

iqdecay commented Jan 24, 2023

Agreed but I feel like Br1ght0ne is hinting that he maybe has a solution that isn't async and doesn't need the user to know the tx_id.
My question was: is it too urgent for us to wait on the implementation which seems tricky? Or can we wait a bit.

segfault-magnet
segfault-magnet previously approved these changes Feb 2, 2023
hal3e
hal3e previously approved these changes Feb 2, 2023
MujkicA
MujkicA previously approved these changes Feb 4, 2023
@Br1ght0ne Br1ght0ne dismissed stale reviews from MujkicA, hal3e, and segfault-magnet via 8cdac16 February 27, 2023 11:40
@Br1ght0ne Br1ght0ne enabled auto-merge (squash) February 27, 2023 11:41
@Br1ght0ne Br1ght0ne disabled auto-merge March 4, 2023 08:50
@Br1ght0ne Br1ght0ne closed this Apr 10, 2023
@Br1ght0ne Br1ght0ne force-pushed the oleksii/call-response-txid branch from d4d6731 to 00e2ada Compare April 10, 2023 12:50
@Br1ght0ne Br1ght0ne reopened this Apr 10, 2023
@Br1ght0ne Br1ght0ne requested a review from iqdecay April 10, 2023 12:50
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

LGTM, but please add documentation and tests

@Br1ght0ne Br1ght0ne requested a review from iqdecay May 15, 2023 13:33
iqdecay
iqdecay previously approved these changes May 19, 2023
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

nice

packages/fuels-programs/src/contract.rs Outdated Show resolved Hide resolved
packages/fuels-programs/src/contract.rs Outdated Show resolved Hide resolved
packages/fuels-programs/src/script_calls.rs Outdated Show resolved Hide resolved
Co-authored-by: iqdecay <victor@berasconsulting.com>
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

Lgtm

@Br1ght0ne Br1ght0ne enabled auto-merge (squash) May 19, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ergonomic way to fetch the transaction ID(s) from a contract call
6 participants