Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add force_batch to utility pallet #11148

Merged
merged 9 commits into from
May 5, 2022

Conversation

jasl
Copy link
Contributor

@jasl jasl commented Mar 31, 2022

Add force_batch to utility pallet which is useful for you wanna batch a bunch of calls that some of them may failed but failed can be ignored.

polkadot address: 13asnZw4BnT2dsTYU4YTm3GMXySF6ocNov96493SPMNrVd2c
polkadot companion: paritytech/polkadot#5452

@ggwpez
Copy link
Member

ggwpez commented Apr 2, 2022

Very cool! You already have this deployed on the Khala testnet, right? For what are you using it, if I may ask?
A cargo +nightly fmt is needed for the CI and we need to re-benchmark it.

The code is very close the the original batch extrinsic. Any general ideas on this @bkchr, @shawntabrizi ?
PS: Does this break the API?

@ggwpez ggwpez added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Apr 2, 2022
@ggwpez ggwpez added this to In progress in Runtime via automation Apr 2, 2022
@jasl
Copy link
Contributor Author

jasl commented Apr 2, 2022

Very cool! You already have this deployed on the Khala testnet, right? For what are you using it, if I may ask? A cargo +nightly fmt is needed for the CI and we need to re-benchmark it.

The code is very close the the original batch extrinsic. Any general ideas on this @bkchr, @shawntabrizi ? PS: Does this break the API?

I forgot to click "comment" 😄

You already have this deployed on the Khala testnet, right?

Yes! we use this for a long time, I forgot to contribute to Substrate...

For what are you using it

We have serveral usecase, for example:

  • Khala chain is a control plane for off-chain workers (aka pruntime or miner), so people will group control their workers, some of miners already stopped or can't start for some reason (e.g. stake isn't enough), so some of calls fail is OKay for users, just pick those failed and retry
  • Workers should respond messages, they will submit their responses to the chain, sometimes workers duplicate respond a message, it's OK, and we don't want other responses drop

A cargo +nightly fmt is needed for the CI and we need to re-benchmark it.

Done

Overrall, this is another version of batch, now we have 3 types of batch:

  • batch run calls until meets an error
  • batch_all like transaction that all calls should success
  • batch_try run calls and don't care error

@bkchr
Copy link
Member

bkchr commented Apr 2, 2022

The naming isn't great. Try implies that you try to execute something and bail on an error. So, this naming would fit better to the original dispatchable. We should rename the new one to convey its implementation details better to the user.

@jasl
Copy link
Contributor Author

jasl commented Apr 2, 2022

The naming isn't great. Try implies that you try to execute something and bail on an error. So, this naming would fit better to the original dispatchable. We should rename the new one to convey its implementation details better to the user.

could you give a suggest? it's should useful for batch a bunch of discrate calls, we also not satisfied its naming, but I don't have better idea.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Code seems to look fine, but I would call this force_batch, since you are forcing the execution of all calls, even if one fails?

@bkchr
Copy link
Member

bkchr commented Apr 4, 2022

force_batch sounds good to me :)

@jasl
Copy link
Contributor Author

jasl commented Apr 4, 2022

Name changed.

@jasl jasl changed the title Add batch_try to utility pallet Add force_batch to utility pallet Apr 4, 2022
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

I will put this in the audit queue soon, will probably take a while.

frame/utility/src/lib.rs Outdated Show resolved Hide resolved
frame/utility/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me.

frame/utility/src/lib.rs Outdated Show resolved Hide resolved
Runtime automation moved this from In progress to Needs Audit Apr 6, 2022
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Should await audit.

@kianenigma kianenigma removed the A0-please_review Pull request needs code review. label Apr 16, 2022
Co-authored-by: Louis Merlin <hello@louismerl.in>
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Apr 28, 2022

User @louismerlin, please sign the CLA here.

@jasl
Copy link
Contributor Author

jasl commented May 4, 2022

@bkchr companion added.

@shawntabrizi
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#5452

@shawntabrizi
Copy link
Contributor

@jasl if you would like a small tip in KSM or DOT, please include your address in the PR description.

@bkchr
Copy link
Member

bkchr commented May 4, 2022

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#5452

@jasl
Copy link
Contributor Author

jasl commented May 5, 2022

@jasl if you would like a small tip in KSM or DOT, please include your address in the PR description.

wow thanks!

@jasl
Copy link
Contributor Author

jasl commented May 5, 2022

bot merge

maybe I do things wrong...

follow the guide (https://github.com/paritytech/substrate/blob/master/docs/CONTRIBUTING.adoc#updating-polkadot-as-well) I've done step 1 - 5

use diener patch --crates-to-patch ../substrate --substrate to patch Polkadot, so the PR includes patch dependencies redirect to my local folder.

@bkchr
Copy link
Member

bkchr commented May 5, 2022

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#5452

@bkchr
Copy link
Member

bkchr commented May 5, 2022

/tip small

@substrate-tip-bot
Copy link

A small tip was successfully submitted for jasl (13asnZw4BnT2dsTYU4YTm3GMXySF6ocNov96493SPMNrVd2c on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

@bkchr
Copy link
Member

bkchr commented May 5, 2022

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#5452

@bkchr
Copy link
Member

bkchr commented May 5, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit f996a3a into paritytech:master May 5, 2022
Runtime automation moved this from Needs Audit to Done May 5, 2022
@jasl jasl deleted the port-batch_try branch May 5, 2022 12:29
@shawntabrizi shawntabrizi moved this from Done to Archive in Runtime May 5, 2022
godcodehunter pushed a commit to sensoriumxr/substrate that referenced this pull request Jun 22, 2022
* Add batch_try to utility pallet

* lint

* rename utility.batch_try -> utility.force_batch

* Remove un-needed index field for utility.ItemFailed event

* Remove indexes of utility,BatchCompletedWithErrors

* Apply suggestions from code review

Co-authored-by: Louis Merlin <hello@louismerl.in>

Co-authored-by: Louis Merlin <hello@louismerl.in>
Co-authored-by: Bastian Köcher <info@kchr.de>
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Add batch_try to utility pallet

* lint

* rename utility.batch_try -> utility.force_batch

* Remove un-needed index field for utility.ItemFailed event

* Remove indexes of utility,BatchCompletedWithErrors

* Apply suggestions from code review

Co-authored-by: Louis Merlin <hello@louismerl.in>

Co-authored-by: Louis Merlin <hello@louismerl.in>
Co-authored-by: Bastian Köcher <info@kchr.de>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Add batch_try to utility pallet

* lint

* rename utility.batch_try -> utility.force_batch

* Remove un-needed index field for utility.ItemFailed event

* Remove indexes of utility,BatchCompletedWithErrors

* Apply suggestions from code review

Co-authored-by: Louis Merlin <hello@louismerl.in>

Co-authored-by: Louis Merlin <hello@louismerl.in>
Co-authored-by: Bastian Köcher <info@kchr.de>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants