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: cannot fetch mint transactions #1203

Merged
merged 10 commits into from
Nov 17, 2023
Merged

bug: cannot fetch mint transactions #1203

merged 10 commits into from
Nov 17, 2023

Conversation

segfault-magnet
Copy link
Contributor

closes #1199

Mint transactions can now be fetched through the Provider. Implemented what made sense for Mint, left the rest as unimplemented!. Without some hefty redesign don't see what other option is there but to violate the LSP (L for Liskov).

Decided to wrap Mint, so that users may extract the id, outputs, etc, without having to know about the various traits from fuel.

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.

@segfault-magnet segfault-magnet added the bug Something isn't working label Nov 14, 2023
@segfault-magnet segfault-magnet self-assigned this Nov 14, 2023
}
}
}

fn unsupported_mint_operation(operation: &str) -> ! {
unimplemented!("Calling `{operation}` on Mint transactions is unsupported!");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it'd be better to just return an error instead of panicking? Imo we shouldn't have any public apis exposed to users that allow them to cause a panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started a discussion about not implementing this trait at all. If that for some reason isn't accepted, I'll change the trait so that methods that would have passed normally on a Script or Create will fail in the case of a Mint transaction.

Copy link
Contributor

@MujkicA MujkicA left a comment

Choose a reason for hiding this comment

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

Does Mint have to implement Transaction? We use the trait as a common interface for Create and Script when submitting them to the provider and AFAIK submitting Mint currently makes no sense.
IMO it's better to not implement Transaction here since it will also guard against passing the tx where it doesn't belong.

On the other hand a transaction that is not Transaction could be confusing. Maybe renaming the trait to something like SubmittableTransaction would make sense.

@Voxelot
Copy link
Member

Voxelot commented Nov 15, 2023

In the VM we use a trait called ExecutableTransaction so that it only accepts create or script txs

@segfault-magnet
Copy link
Contributor Author

segfault-magnet commented Nov 15, 2023

Does Mint have to implement Transaction? We use the trait as a common interface for Create and Script when submitting them to the provider and AFAIK submitting Mint currently makes no sense.

If you take a closer look you'll notice that Mint doesn't implement Transaction. Rather it is the TransactionType enum that implements it. Its Transaction implementation just delegates to the underlying transaction. But how do you delegate the methods that only make sense for Create of Script to the Mint transaction? This is the breaking of the LSP I mentioned above.

You also cannot implement something like ExecutableTransaction since you'll have the same problem -- what to do with the Mint variant?

The only solution is not to implement anything for this enum and have it just be a bare enum, used so that we can return Create, Script, or Mint transactions requested by the user via the get_transactions endpoint of the Provider.

I don't see the methods of Transaction being used with a TransactionType -- I think the goal was to circumvent the issues of the trait not being object safe by using an enum and just delegating to the variants. This, as mentioned above, falls apart with the introduction of Mint.

I suggest we remove the Transaction impl from the TransactionType and have users match, extract the underlying transaction and then call whatever they like on it.

@MujkicA @Voxelot

@segfault-magnet
Copy link
Contributor Author

Ended up removing the Transaction implementation from the TransactionType. Now it is just a simple wrapper around the three transactions.

digorithm
digorithm previously approved these changes Nov 16, 2023
hal3e
hal3e previously approved these changes Nov 16, 2023
Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

LGTM!

@hal3e hal3e changed the title bug: Cannot fetch mint transactions bug: cannot fetch mint transactions Nov 16, 2023
Br1ght0ne
Br1ght0ne previously approved these changes Nov 17, 2023
Salka1988
Salka1988 previously approved these changes Nov 17, 2023
iqdecay
iqdecay previously approved these changes Nov 17, 2023
@segfault-magnet segfault-magnet dismissed stale reviews from iqdecay and Salka1988 via cdbee7c November 17, 2023 23:11
@segfault-magnet segfault-magnet dismissed stale reviews from Br1ght0ne, hal3e, and digorithm via cdbee7c November 17, 2023 23:11
@digorithm digorithm merged commit b2676a0 into master Nov 17, 2023
38 checks passed
@digorithm digorithm deleted the bug/fetch_mint_tx branch November 17, 2023 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot fetch Mint transactions through Provider
8 participants