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

Clean up extrinsic params builder #405

Merged
merged 17 commits into from
Jan 6, 2023

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Dec 30, 2022

Changes the following:

  • Removed types AssetTipExtrinsicParamsBuilder and PlainTipExtrinsicParamsBuilder. They are simply unnecessary and cause confusion. The api already sets the needed types (Tip, Index and Hash).
  • Renamed ExtrinsicParamsBuilder to AdditionalParams. Reasoning: the name "Builder" suggests that it builds "ExtrinsicParams", but it does not. It only ever built the OtherParams. But a builder seems a little over the top anyway. So a simple ...AdditionalParams should do it.
  • Renamed Base or Default to Generic because they are compatible with most substrate / polkadot nodes and are generic over Tip and Hash. Base did not fit IMO because that implicates that it is extensible. But it is not. One would need to redefine the Params in case something is changed on the node side.

closes #400

@haerdib haerdib self-assigned this Dec 30, 2022
@haerdib haerdib changed the title Bh/400 clean up extrinsic params builder Clean up extrinsic params builder Jan 3, 2023
@haerdib haerdib force-pushed the bh/400-clean-up-extrinsic-params-builder branch from 9547293 to 73c0fd5 Compare January 3, 2023 13:53
@haerdib haerdib added F7-enhancement Enhances an already existing functionality E2-breaksapi labels Jan 3, 2023
@haerdib haerdib marked this pull request as ready for review January 4, 2023 09:33
@haerdib haerdib requested a review from Niederb January 4, 2023 09:33
@haerdib haerdib removed the F7-enhancement Enhances an already existing functionality label Jan 4, 2023
Copy link
Contributor

@Niederb Niederb left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nitpicky suggestions.
Regarding Polkadot vs SubstrateDefault I don't have a strong opinion. I think I would leave it as is for now.

src/api/api_client.rs Outdated Show resolved Hide resolved
src/api/api_client.rs Outdated Show resolved Hide resolved
src/api/api_client.rs Outdated Show resolved Hide resolved
@clangenb
Copy link
Collaborator

clangenb commented Jan 4, 2023

Nice looks in general good great that you noticed that we can remove the specific builder definitions! I have a few remarks about the terminology:

Renamed Base or Default to Polkadot because they are polkadot-node compatible (and default substrate node). Base did not fit IMO because that implicates that it is extensible. But it is not. One would need to redefine the Params in case something is changed on the node side.

Base might be a bit off, but I think polkadot is wrong because it implies that the params are less generic than they actually are. polkadot-node does not allow AssetTip, while the PolkadotExtrinsicParams do allow this. This is why I chose base, not because it is extensible, but because it is the generic version that can be concretized to polkadot/kusmama/substrate_default or statemine/statemint etc.

So from my point of view we can either name it: ExtrinsicParams (then we need to rename the the ExtrinsicParams trait to ExtrinsicParamsTrait), or we call it GenericExtrinsicParams.

@haerdib
Copy link
Contributor Author

haerdib commented Jan 5, 2023

Renamed to Generic - I think that fits the purpose. Thanks for the input :)

Copy link
Contributor

@Niederb Niederb left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the naming! Looks good to me!

@haerdib haerdib merged commit bf59f6b into master Jan 6, 2023
@haerdib haerdib deleted the bh/400-clean-up-extrinsic-params-builder branch January 6, 2023 07:12
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.

Investigate clean up of ExtrinsicParamsBuilder
3 participants