Fix #387 - Generalize the Event and Error of tasks by separating execution from triggering and rescheduling

[imstar15]

Fix the issue of auto compound task:

1. When there’s not enough fee to pay for this execution, fail it with InsufficientFee error but do not stop the recurring tasks. At the next execution, we check it again to see if there’s enough fee for execution.

2. The task will stop when there’s no staking delegation within the user’s wallet. That means the user has unstaked all from collators.

Changes in pallet-parachain-staking:
https://github.com/OAK-Foundation/moonbeam/pulls

[v9n]

is it safe to including the entire error message? will they ever grow big? i think unlikely but maybe we should cap length

[chrisli30]

Discussed with Charlse. In this line of code, error_message doesn’t contain more info than the error so we need to pass in variables to make it more meaningful. Let’s not bother the length for now. We can prune them later.

@v9n, there are two ways to go with this error_message,
First, we can add full information here, e.g. "delegator:<delegator_wallet_address>. collator: <collator_wallet_address>", however, the two wallet information can be inferred and don’t really needed in this case, because delegator is the wallet who created this task and collator is specified during task creation.

Adding the full error_message will help live debugging, but as you said will increase output and maybe blockchain data size. What do you recommend, should we log the full message or leave it blank? Could leaving it blank really save block space (for example, maybe the size of the event is decided by its largest parameter)?

[v9n]

yes, the log message absolutely userful when debugging.

I don't think log/event is saving into block space(at least in EVM they are stored separaly because log doesn't require consensus).

Let emit the full message IMHO. I was just want to make sure we don't accidently store MB of data due to generated string.

[chrisli30]

@imstar15, as Vinh said, let’s add the variables to error_message in the below format, and update the tests accordingly.
"delegator:<delegator_wallet_address>, collator: <collator_wallet_address>"

[chrisli30]

Comments for the new test cases are clear, 👍 however, there are a few more comments and questions.

Besides, we need to update the Documentation regarding this API on Docs website, https://docs.oak.tech/docs/yield-boost-auto-restaking/. @imstar15 please work on the documentation after this PR is approved and merged.

1. The skip logic needs to be added to the page
2. Error code and messages need to be listed on the page.

[imstar15]

In Rust, the format! macro requires the formatting string to be a string literal. We cannot use constants or static variables directly as the formatting string. To address this, we can use a macro to extract the formatting string.

[chrisli30]

I don’t think we need an extra hex library to encode the wallets. Can you write down the actual message of the error_message here? Is the wallet address still human readable after hex encoding?

On second thought, if we were to add more details, we should always print out delegator and collator for all AutoCompoundDelegatorStake events in a way that is easier to parse.

For example,
AutoCompoundDelegatorStakeSucceeded

• taskId
• amount
• collator

AutoCompoundDelegatorStakeSkipped

• taskId
• amount
• collator
• message: Unable to pay fees for the next execution
• value:

The balance reason needs an explanation, as in what was the balance of the wallet, so we add that information by adding an extra field to this event. The challenge is to find a common method to add extra information to a message, or an error. Let me ask round to find out the best practice to extend the Error definition. @v9n any recommendations? We need to standardize the format of Success and Error events.

AutoCompoundDelegatorStakeFailed

• taskId
• amount
• collator
• error: DelegationNotFound

Since there’s already a collator information included, we don’t need any extra fields for this AutoCompoundDelegatorStakeFailed error

[imstar15]

After undergoing hex processing, it will become an account ID string.
For example: 0x3830ef2fdb02d711afe91c830737375e32ac9904886266897f361b5a2ec13a39

[v9n]

is there a reason we cannot use the wallet address of delegator or collator but have to hexing them? because hexing them make our original purpose of easy debugging become harder. hexing is one way so cannot easy reverse without a utility convert function somewhere.

also, If it's hard/inconvenience to format into the string maybe we can just return a Vec such as

deposit_event(FailedEvent {
  delegator: Vec<u8> address,
  collator: ...,
  error_mesage: ...
  error: ...
}

This make it easiy to parse on the client when indexing event, but make it hard to add more arbitrary data because we need to change the event shape. with the error_mesage as string/Vec it was easy to put add/any info down the life.

[chrisli30]

Yes, wallet addresses need to be printed out, but not hex or accountId. @imstar15

[chrisli30]

The macro_export approach looks a little clumsy to me, so we need to find a better solution for Events and Error.

[chrisli30]

I don’t think we need an extra hex library to encode the wallets. Can you write down the actual message of the error_message here? Is the wallet address still human readable after hex encoding?

On second thought, if we were to add more details, we should always print out delegator and collator for all AutoCompoundDelegatorStake events in a way that is easier to parse.

For example,
AutoCompoundDelegatorStakeSucceeded

• taskId
• amount
• collator

AutoCompoundDelegatorStakeSkipped

• taskId
• amount
• collator
• message: Unable to pay fees for the next execution
• value:

The balance reason needs an explanation, as in what was the balance of the wallet, so we add that information by adding an extra field to this event. The challenge is to find a common method to add extra information to a message, or an error. Let me ask round to find out the best practice to extend the Error definition. @v9n any recommendations? We need to standardize the format of Success and Error events.

AutoCompoundDelegatorStakeFailed

• taskId
• amount
• collator
• error: DelegationNotFound

Since there’s already a collator information included, we don’t need any extra fields for this AutoCompoundDelegatorStakeFailed error

[v9n]

The macro_export approach looks a little clumsy to me, so we need to find a better solution for Events and Error.

@chrisli30 @imstar15 just share my though here. Instead of a String, we can may build the string with Vector?

Example:

 //  'delegator'
const DELEGATOR_MSG = [100, 101, 108, 101, 103, 97, 116, 111, 114];;
 // collator
const COLLATOR_MSG = [32, 99, 111, 108, 108, 97, 116, 111, 114];

fn build_error_message(collator: vec, delagator vec) -> Vec<u8> {
  let s = DELEGATOR_MSG.to_vec(),
  s.extend(delegator);
  s.extend(COLLATOR_MSG.to_vec()); 
  s.extend(collator);
  s
}

I'm not sure if it's more clean but at least we don't use macro, we can also add any thing there and don't need to rely on format! std. We can also limit the length here for build_error_message as well etc.

[v9n]

I think we should still keep this verify block. i had a similar code in here we can use to get the right task id

https://github.com/OAK-Foundation/OAK-blockchain/blob/917cb56330ece0ac6fc8c5b79a804ac04ec39e8a/pallets/automation-time/src/benchmarking.rs#L221-L227

[imstar15]

Because run_dynamic_dispatch_action does not throw events, it only returns error. Therefore, I checked the error it returns. Since error variable cannot be used in the verify block, I did not include these checks in the verify block

Refers to:
https://github.com/paritytech/substrate/blob/fbddfbd76c60c6fda0024e8a44e82ad776033e4b/frame/state-trie-migration/src/lib.rs#L964

.

[v9n]

ic, that makes sense.

[v9n]

Per OIP #387 I don't think we intent to include abort_errors on TaskTriggered @chrisli30 ? It is redundant because it is the same array we clone from task.abort_errors.

My understanding is when a task execution failed, we compared the error it throw with a pre-defined list(or maybe user input in the future) in abort_errors, if matched, we stop re-schedule for the next run. so abort_error should be a single error of the particular error that match one of error in the task.abort_errors, and will be include in TaskNotSchedule/TaskFailedSchedule ?

[chrisli30]

Good call,

TaskTriggered doesn’t need this array cuz it’s too noisy.

Neither does TaskNotRescheduled need it, because it already contains the actual error that caused the NotRescheduled event. We can always look up the task creation event to verify that the causing error is in the abort_errors array.

TaskRescheduleFailed doesn’t need this because that error is most likely to be caused by block full and not relate to abort_errors.

[imstar15]

Fixed

[v9n]

is there a reason we need to pass task_id and task separately? inside the task we already had the task_id.

[imstar15]

Good call!
Fixed.

[chrisli30]

We don’t need this CallCannotBeDecoded event.

When CallCannotBeDecoded error happens, the below two Events need to capture it

1. TaskExecutionFailed
2. TaskNotRescheduled.

[imstar15]

Fixed

[chrisli30]

Looks good enough to me.

[chrisli30]

Maybe I’m wrong, but Is this assert_last_event util function recently added by @v9n? Should we keep or remote the function?

[v9n]

the util function already removed in this PR. It was being used only in benchmark file. but with the change in this PR, we move away from events -> return dispatch error so now we don't need to assert the event anymore.

I still think it's useful to keep the function around but at the same time it's one liner and currently we don't use it at al anywhere so I think ok to remove for now and in the future if we ever has that need we can just whipe up it quickly.

#[cfg(any(feature = "std", feature = "runtime-benchmarks", test))]
pub fn assert_last_event(event: RuntimeEvent) {
	assert_eq!(events().last().expect("events expected"), &event);
}

[imstar15]

Yes, we don't need it now.

If left there, a warning will be generated. Perhaps this warning can be removed through some annotations.

[imstar15]

I revert assert_last_event function and add #[allow(dead_code)] annotation on it