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

Expose signature of the block, if any in GraphQL #770

Merged
merged 11 commits into from
Nov 12, 2022

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Nov 11, 2022

Previously the producer of the block was a part of the header. But after change it is not. Added exposing of all information required to get it

@@ -79,6 +84,19 @@ impl Block {
&self.header
}

async fn signature(&self, ctx: &Context<'_>) -> Option<Signature> {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have something more generic than signature here, as PoS will have multiple signatures etc. Can we reuse the fuel block consensus type instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course, we can=) I did it as a signature because it is easy to use. The question do we want to bring the complexity now or later=)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After a private discussion, we can add complexity now and add an additional block_producer method to the Block.

@xgreenx xgreenx requested a review from Voxelot November 11, 2022 02:38
@@ -19,6 +19,7 @@ members = [
"fuel-tests",
"fuel-txpool",
"xtask",
"version-compatibility",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Voxelot wrote in the slack:

Im thinking we should test if client will still work against 0.14.1. Since it's a new field it'd be nice if an older version of the server just ignored the request, but we should double check that this won't suddenly break the SDK etc

So I added tests to verify version compatibility between 0.14.1 and 0.14.2

if let TransactionStatus::Success { block_id, .. } =
transaction_response.unwrap().status
{
// TODO: We need to decide is that breaking change or not. For now, the test fails,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If fuel-client is 0.14.2 but fuel-core is 0.14.1 the test fails with "Unknown field 'consensus'".
We have 4 variants on how to fix that=)

  1. Make a breaking change 0.15.0.
  2. Extract consensus from Block into a separate method and on the client side try to fetch that part if it is possible in a separate request.
  3. Do two requests, one with a new version of the block, if it fails, send it with the old version=)
  4. Add a new method to the Block in fuel-client to get the consensus field by passing FuelClient=)

Copy link
Member

@Voxelot Voxelot Nov 11, 2022

Choose a reason for hiding this comment

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

Option 1. would be the cleanest, as fuel-client 0.15.0 would still be compatible with fuel-core 0.14.1.

@Voxelot Voxelot added the breaking A breaking api change label Nov 11, 2022
@xgreenx xgreenx merged commit b5fdd19 into master Nov 12, 2022
@xgreenx xgreenx deleted the feature/expose-signature branch November 12, 2022 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking api change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants