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

Investigate: can the tip type be detected via metadata or Runtime? #344

Closed
haerdib opened this issue Nov 28, 2022 · 13 comments
Closed

Investigate: can the tip type be detected via metadata or Runtime? #344

haerdib opened this issue Nov 28, 2022 · 13 comments
Labels
F7-enhancement Enhances an already existing functionality Q5-involved Will take some time to fix this

Comments

@haerdib
Copy link
Contributor

haerdib commented Nov 28, 2022

Currently, it has to be manually chosen which type of tips are used in the runtime (AssetTip, PlainTip). That's a hassle. Shouldn't it be possible to determine that via the metadata?

@haerdib haerdib added F7-enhancement Enhances an already existing functionality Q3-substantial labels Nov 28, 2022
@haerdib
Copy link
Contributor Author

haerdib commented Nov 28, 2022

Or maybe via Runtime (via PR #340) as Rust does not like type determination during runtime.

@haerdib haerdib changed the title Can the tip type be detected via metadata? Can the tip type be detected via metadata or Runtime? Nov 28, 2022
@haerdib haerdib added Q5-involved Will take some time to fix this and removed Q3-substantial labels Nov 28, 2022
@haerdib
Copy link
Contributor Author

haerdib commented Nov 28, 2022

I have investigated a little - and got to the following result:

  • I think, it is not possible to deduct the Tip type via the Runtime. The AdditionalSigned is too generic. However, it is possible to get the SignedExtensions type from the Runtime:
pub struct SignedExtraWrapper<Runtime>
where
	Runtime: GetRuntimeBlockType + frame_system::Config,
	<Runtime::RuntimeBlock as Block>::Extrinsic: ExtrinsicMetadata,
{
	pub extra: <<Runtime::RuntimeBlock as Block>::Extrinsic as ExtrinsicMetadata>::SignedExtensions, 
	//  ^^^ That's the Extra which implements the Trait SignedExtension, e.g. the function:
	fn additional_signed(&self) -> Result<Self::AdditionalSigned, TransactionValidityError>;
}

And that might be enough. The compose macors would need to be adapted.

  • Getting the Tip Type via metadata might be possible. What's certainly possible though, is retrieving the AddtionalSigned type, with the SignedExtensionMetadatatype.

Useful substrate locations:

@haerdib
Copy link
Contributor Author

haerdib commented Dec 29, 2022

Related issue : paritytech/substrate#12929

Via metadata (V14) it can be detected if the pallet_asset_tx_payment is imported into the runtime. And I believe, with quite some certainty, one can say if that's the case, then the fees will be paid with that pallet and so will the tips be. But I guess that not 100% sure, because one could simply define that pallet but not use it at all.
This might be better once V15 is published.

Conclusion: I think it's not worth to invest too much time to this. Let's wait for V15 and hope for a better world. If that takes too long, #406 might be a reasonable solution.

@haerdib haerdib self-assigned this Jan 9, 2023
@haerdib haerdib removed their assignment Jan 10, 2023
@clangenb
Copy link
Collaborator

You can't because encointer does not use the pallet_asset_tx_payment, but we use CommunityIdentifier as asset id, which is not exposed in the metadata AFAIK.

@haerdib
Copy link
Contributor Author

haerdib commented Mar 23, 2023

Happy news: Metadata v15 incoming soon! (checkout: paritytech/substrate#13287) 🥳

@haerdib haerdib changed the title Can the tip type be detected via metadata or Runtime? Investigate: can the tip type be detected via metadata or Runtime? Nov 14, 2023
@haerdib
Copy link
Contributor Author

haerdib commented Nov 14, 2023

Metadata v15 is released, so this might not be blocked anymore.

@haerdib
Copy link
Contributor Author

haerdib commented Feb 9, 2024

I've looked into it once again and came to the following conclusion: Yes, it would be possible to detect the tip during runtime with the metadata, but that would complicate a lot of things:

  • Type safety: To allow generic tips, without allocating the tip type during compilation time, we would need to remove type safety during compilation.
  • While we can detect the "standard" tips according to their identifiers (ChargeTransactionPayment, ChargeAssetTxPayment, ..), I don't see how we can do that with self defined pallet tips (as long as the order of the SignedExtra is not fixed, which it is not AFAIK). So users having their own tip-pallet would have to set something before compilation, either way.

So long it's not a pressing user issue which has actual use cases, I don't see enough benefit on setting the Tip during runtime.
What we could easily do however is to check the set config against the metadata and if there's a mismatch we can print an error or warning.

@haerdib
Copy link
Contributor Author

haerdib commented Feb 9, 2024

@masapr what do you think?

@clangenb
Copy link
Collaborator

At some point, there might be some relevant information in this issue: leonardocustodio/polkadart#359

@masapr
Copy link
Collaborator

masapr commented Feb 22, 2024

@masapr what do you think?

I agree. It sounds like too much of a hassle.

@haerdib have you checked @clangenb's comment? Is this of any help?

@haerdib
Copy link
Contributor Author

haerdib commented Feb 23, 2024

According to my understanding, they solve the problem by adding the extra input customSignedExtensions which may be the assetId or something else:

    final payloadWithCustomSignedExtension = SigningPayload(
      method: encodedCall,
      specVersion: specVersion,
      transactionVersion: transactionVersion,
      genesisHash: genesisHash,
      blockHash: blockHash,
      blockNumber: blockNumber,
      eraPeriod: 64,
      nonce: 0, // Supposing it is this wallet first transaction
      tip: 0,
      customSignedExtensions: <String, dynamic>{
        'PrevalidateAttests': 0, // A custom Signed Extensions
      },
    );

I'm not sure if that's easier than adapting the Config. However, it might be an extension possibility: Add a dynamic signing option for the users. Not sure about the use case though, as long as there's no cli. What I also like is the dynamic setting of the additional signed - not like our hardcoded part here:

pub type GenericAdditionalSigned<Hash> = ((), u32, u32, Hash, Hash, (), (), ());

I believe we could start by adding sensible runtime checks against the metadata and print warnings if there's a mismatch. That way, the users will know what to adapt without first investigating some strange runtime errors.

@masapr
Copy link
Collaborator

masapr commented Feb 23, 2024

Yes, I think giving good warnings is probably the best in this case. Before we make it too complicated.

@haerdib
Copy link
Contributor Author

haerdib commented Feb 26, 2024

Closing this issue in favour of the two follow-up issues #730 and #731. Please reopen if you disagree.

@haerdib haerdib closed this as completed Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F7-enhancement Enhances an already existing functionality Q5-involved Will take some time to fix this
Projects
None yet
Development

No branches or pull requests

3 participants