Return reason of why proof cant be generated #2258

rafal-ch · 2024-09-26T16:06:28Z

Description

message_proof() API is modified to return either the proof or an error with the description why the proof couldn't be created. Previously the reason was discarded by returning Ok(None).

Before requesting review

I have reviewed the code myself

…nerated

crates/fuel-core/src/query/message.rs

acerone85 · 2024-09-27T13:58:27Z

In general my understanding is that breaking changes in graphQL are discouraged, and instead you can add a "no_proof_reason" field to the API. Third-party Applications will be able to use the service, but can update their clients to request the "no_proof_reason" field and handling it accordingly.

Overall I don't have a strong opinion, and a breaking change might be okay in this case (I don't have enough context to say whether this is the case), just thought of mentioning the alternative

xgreenx · 2024-09-27T16:08:24Z

crates/client/assets/schema.sdl

@@ -970,7 +970,7 @@ type Query {
 		"""
 		owner: Address,		first: Int,		after: String,		last: Int,		before: String
 	): MessageConnection!
-	messageProof(transactionId: TransactionId!, nonce: Nonce!, commitBlockId: BlockId, commitBlockHeight: U32): MessageProof
+	messageProof(transactionId: TransactionId!, nonce: Nonce!, commitBlockId: BlockId, commitBlockHeight: U32): MessageProof!


@acerone85 @luizstacio @LuizAsFight is it a breaking change for you?

If yes, we can just always return Some(proof).

This is not a breaking change for the TS SDK, we have null handling but it'll just be redundant.

Not sure about the FE apps though as they do have some custom GQL logic.

no breaking change on FE side

Thanks for the feedback, I'm removing the "breaking" label then.

xgreenx · 2024-09-27T16:10:54Z

crates/fuel-core/src/query/message.rs

+    // Check if we found a leaf.
+    let Some(proof_index) = proof_index else {
+        return Err(anyhow::anyhow!("unable to find leaf").into())
+    };
+
+    // Get the proof set.
+    let Some((_, proof_set)) = tree.prove(proof_index) else {
+        return Err(anyhow::anyhow!("unable to get proof set").into());
+    };


It would be nice to get more information during which operation you we got an error

We can consider getting rid of the ok() here and provide details carried over in MerkleTreeError.

Is this what you meant?

As agreed, the comment was about using more user-friendly error messages. Updated in Updated in 2199e87.

xgreenx · 2024-09-27T16:11:17Z

crates/fuel-core/src/query/message.rs

+    let Some(block) = database
+        .block(&block_height)
        .into_api_result::<CompressedBlock, StorageError>()?


I like old naming for vairables more=)

message_block and message_block_height are back in 0c2dbc5

xgreenx

What do you think about starting the error message with capital letters?=)

rafal-ch · 2024-09-30T08:01:41Z

What do you think about starting the error message with capital letters?=)

I prefer lowercase mostly because they are usually a part of bigger sentences - and it reads better without a capital letter in the middle of a message.
Also, I think that's the convention Rust language devs use.

But after all, there's a merit in having consistent error messages. I see that we mostly use uppercase with just a few exceptions.

…`let` bindings

…f_cant_be_generated' into 1394_return_reason_of_why_proof_cant_be_generated

xgreenx · 2024-10-01T06:52:49Z

crates/fuel-core/src/query/message.rs

-        _ => return Ok(None),
+        .into_api_result::<TransactionStatus, StorageError>()?
+    else {
+        return Err(anyhow::anyhow!("unable to obtain block height").into())


Suggested change

return Err(anyhow::anyhow!("unable to obtain block height").into())

return Err(anyhow::anyhow!("Unable to obtain the message block height").into())

Updated in 2199e87

xgreenx · 2024-10-01T06:53:04Z

crates/fuel-core/src/query/message.rs

-        Some(r) => r,
-        None => return Ok(None),
+    let Some(data) = data else {
+        return Err(anyhow::anyhow!("output message doesn't contain any `data`").into())


Suggested change

return Err(anyhow::anyhow!("output message doesn't contain any `data`").into())

return Err(anyhow::anyhow!("Output message doesn't contain any `data`").into())

Updated in 2199e87

xgreenx · 2024-10-01T06:53:37Z

crates/fuel-core/src/query/message.rs

-        });
+        })
+        .ok_or::<StorageError>(
+            anyhow::anyhow!("desired `nonce` missing in transaction receipts").into(),


Suggested change

anyhow::anyhow!("desired `nonce` missing in transaction receipts").into(),

anyhow::anyhow!("Desired `nonce` missing in transaction receipts").into(),

Updated in 2199e87

xgreenx · 2024-10-01T06:54:11Z

crates/fuel-core/src/query/message.rs

-) -> StorageResult<Option<MessageProof>> {
-    // Check if the receipts for this transaction actually contain this message id or exit.
-    let receipt = database
+) -> StorageResult<MessageProof> {


I think the error should be anyhow since part of the error are not from the storage

I agree the error structure is a little off. We might also take this opportunity and reconsider having Other(anyhow::Error) variant inside the fuel_core_storage::Error enum.

I'm keen to create a follow-up issue for this.

Follow-up: #2291

xgreenx · 2024-10-01T06:54:33Z

crates/fuel-core/src/query/message.rs

-        Some(t) => t.into_inner(),
-        None => return Ok(None),
+    else {
+        return Err(anyhow::anyhow!("unable to get block from database").into())


Suggested change

return Err(anyhow::anyhow!("unable to get block from database").into())

return Err(anyhow::anyhow!("Unable to get the message block from the database").into())

Updated in 2199e87

xgreenx · 2024-10-01T06:54:46Z

crates/fuel-core/src/query/message.rs

-        None => return Ok(None),
+    else {
+        return Err(
+            anyhow::anyhow!("unable to get commit block header from database").into(),


Suggested change

anyhow::anyhow!("unable to get commit block header from database").into(),

anyhow::anyhow!("Unable to get commit block header from database").into(),

Updated in 2199e87

xgreenx · 2024-10-01T06:55:32Z

crates/fuel-core/src/query/message.rs

-    let verifiable_commit_block_height =
-        block_height.pred().expect("We checked the height above");
+    let Some(verifiable_commit_block_height) = commit_block_header.height().pred() else {
+        return Err(anyhow::anyhow!("can not look beyond the genesis block").into())


Suggested change

return Err(anyhow::anyhow!("can not look beyond the genesis block").into())

return Err(anyhow::anyhow!("Impossible to generate proof beyond the genesis block").into())

Updated in 2199e87

xgreenx · 2024-10-01T06:58:12Z

crates/fuel-core/src/query/message.rs

 }

 fn message_receipts_proof<T: MessageProofData + ?Sized>(
    database: &T,
    message_id: MessageId,
    message_block_txs: &[Bytes32],
-) -> StorageResult<Option<MerkleProof>> {
+) -> StorageResult<MerkleProof> {
    // Get the message receipts from the block.
    let leaves: Vec<Vec<Receipt>> = message_block_txs
        .iter()


I think we can remove the usage of into_api_result method in this file now since we have an error

That's right. into_api_result was used to disguise the NotFound error as Ok(None). Currently we either have a proper value or a distinct error.

Updated in 10109c9

xgreenx · 2024-10-01T06:58:49Z

crates/fuel-core/src/query/message.rs

-    }
+    // Check if we found a leaf.
+    let Some(proof_index) = proof_index else {
+        return Err(anyhow::anyhow!("unable to find leaf").into())


Suggested change

return Err(anyhow::anyhow!("unable to find leaf").into())

return Err(anyhow::anyhow!("Unable to find the message receipt in the transaction to generate the proof").into())

Updated in 2199e87

xgreenx · 2024-10-01T07:00:21Z

crates/fuel-core/src/query/message.rs

+
+    // Get the proof set.
+    let Some((_, proof_set)) = tree.prove(proof_index) else {
+        return Err(anyhow::anyhow!("unable to get proof set").into());


Suggested change

return Err(anyhow::anyhow!("unable to get proof set").into());

return Err(anyhow::anyhow!("Unable to generate the Merkle proof for the message from its receipts").into());

Updated in 2199e87

rafal-ch · 2024-10-01T14:25:51Z

But after all, there's a merit in having consistent error messages. I see that we mostly use uppercase with just a few exceptions.

For the sake of consistency we'll use the capitalized error messages.

… returns an `Option`

…status

…med a breaking change

xgreenx

We need to change the fuel-core-client side as well.

The change will be breaking because of that. But it is okay to be only breaking from the Rust type system but not from API perspective(old code still should work with new API).

rafal-ch · 2024-10-02T13:26:18Z

We need to change the fuel-core-client side as well.

This commit should handle that: eee16f2

xgreenx

Looks good to me, but let's merge it after release to avoid any potential problems with API=)

acerone85

LGTM. Happy to approve after release.

rafal-ch added 5 commits September 26, 2024 17:31

Return descriptive errors from message_proof()

952dfed

Update comment

522b5aa

Remove call to expect

b7a07ba

message_proof() no longers return an Option

44ee724

Update changelog

266ba4e

rafal-ch self-assigned this Sep 26, 2024

rafal-ch added 3 commits September 27, 2024 11:39

Return more detailed error data from within message_receipts_proof()

f62804d

Update schema because messageProof no longer returns optional

23b720f

Update changelog

7a2c49c

rafal-ch added the breaking A breaking api change label Sep 27, 2024

rafal-ch and others added 2 commits September 27, 2024 11:54

More detailed info in changelog

b09f0cb

Merge branch 'master' into 1394_return_reason_of_why_proof_cant_be_ge…

f6c90ae

…nerated

rafal-ch marked this pull request as ready for review September 27, 2024 11:59

rafal-ch requested review from xgreenx, Dentosal and MitchTurner as code owners September 27, 2024 11:59

rafal-ch requested a review from a team September 27, 2024 12:00

acerone85 reviewed Sep 27, 2024

View reviewed changes

crates/fuel-core/src/query/message.rs Outdated Show resolved Hide resolved

xgreenx reviewed Sep 27, 2024

View reviewed changes

rafal-ch added 3 commits September 30, 2024 10:12

More concise implementation of message_proof() with ok_or()

1956a88

Bring back the explicit message_ prefix in several message-related …

0c2dbc5

…`let` bindings

Merge remote-tracking branch 'upstream/1394_return_reason_of_why_proo…

00cc97f

…f_cant_be_generated' into 1394_return_reason_of_why_proof_cant_be_generated

xgreenx reviewed Oct 1, 2024

View reviewed changes

Use more user-friendly error messages

2199e87

xgreenx previously approved these changes Oct 1, 2024

View reviewed changes

rafal-ch removed the breaking A breaking api change label Oct 2, 2024

rafal-ch added 3 commits October 2, 2024 08:55

Clean-up imports

73f1ff9

Remove calls to into_api_result() since message_proof() no longer…

10109c9

… returns an `Option`

Provide more detailed error message in case of incorrect transaction …

2e551db

…status

rafal-ch dismissed xgreenx’s stale review via 2e551db October 2, 2024 07:05

Move info about PR#2258 from Breaking to Changed as it is not dee…

9a434e0

…med a breaking change

rafal-ch requested a review from a team October 2, 2024 07:13

xgreenx reviewed Oct 2, 2024

View reviewed changes

FuelClient::message_proof() no longer returns an Option

eee16f2

xgreenx reviewed Oct 3, 2024

View reviewed changes

rafal-ch mentioned this pull request Oct 4, 2024

Error should be anyhow since part of the error are not from the storage #2291

Closed

acerone85 reviewed Oct 4, 2024

View reviewed changes

	return Err(anyhow::anyhow!("unable to obtain block height").into())
	return Err(anyhow::anyhow!("Unable to obtain the message block height").into())

	return Err(anyhow::anyhow!("output message doesn't contain any `data`").into())
	return Err(anyhow::anyhow!("Output message doesn't contain any `data`").into())

	anyhow::anyhow!("desired `nonce` missing in transaction receipts").into(),
	anyhow::anyhow!("Desired `nonce` missing in transaction receipts").into(),

	return Err(anyhow::anyhow!("unable to get block from database").into())
	return Err(anyhow::anyhow!("Unable to get the message block from the database").into())

	anyhow::anyhow!("unable to get commit block header from database").into(),
	anyhow::anyhow!("Unable to get commit block header from database").into(),

	return Err(anyhow::anyhow!("can not look beyond the genesis block").into())
	return Err(anyhow::anyhow!("Impossible to generate proof beyond the genesis block").into())

	return Err(anyhow::anyhow!("unable to find leaf").into())
	return Err(anyhow::anyhow!("Unable to find the message receipt in the transaction to generate the proof").into())

Return reason of why proof cant be generated #2258

Are you sure you want to change the base?

Return reason of why proof cant be generated #2258

Conversation

rafal-ch commented Sep 26, 2024 • edited Loading

Description

Before requesting review

acerone85 commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

rafal-ch commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafal-ch commented Oct 1, 2024

xgreenx left a comment

Choose a reason for hiding this comment

rafal-ch commented Oct 2, 2024

xgreenx left a comment

Choose a reason for hiding this comment

acerone85 left a comment

Choose a reason for hiding this comment

rafal-ch commented Sep 26, 2024 •

edited

Loading