Skip to content

Conversation

@bilboquet
Copy link
Contributor

@bilboquet bilboquet commented Feb 14, 2023

  • document all added functions
  • try in sandbox /simulation/labnet
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@bilboquet bilboquet marked this pull request as draft February 14, 2023 15:59
@bilboquet bilboquet self-assigned this Feb 14, 2023
@bilboquet bilboquet linked an issue Feb 14, 2023 that may be closed by this pull request
@bilboquet bilboquet force-pushed the show_operation_execution_status branch 3 times, most recently from 3900c0f to c9263cf Compare February 21, 2023 14:17
@AurelienFT
Copy link
Contributor

Nice job ! Can you fix the CI and place the PR "ready for review" when you want to get a review ? :)

@bilboquet bilboquet force-pushed the show_operation_execution_status branch 4 times, most recently from 7112ddb to 79aad0c Compare February 22, 2023 09:08
@bilboquet bilboquet marked this pull request as ready for review February 22, 2023 09:32
@bilboquet bilboquet force-pushed the show_operation_execution_status branch from 79aad0c to 429ea9c Compare February 22, 2023 10:23
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

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

Nice work ! 💯 I think few simplifications could be done.

@bilboquet bilboquet force-pushed the show_operation_execution_status branch from a8772ac to 9e42a7d Compare February 22, 2023 17:03
@bilboquet bilboquet force-pushed the show_operation_execution_status branch from 9e42a7d to aceae52 Compare February 23, 2023 09:38
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

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

Very nice job ! Will help the community a lot !

@bilboquet bilboquet merged commit 8e12fc3 into testnet_20 Feb 23, 2023
@bilboquet bilboquet deleted the show_operation_execution_status branch February 23, 2023 10:14
@AurelienFT AurelienFT mentioned this pull request Feb 27, 2023
bors bot added a commit that referenced this pull request Mar 1, 2023
3489: Testnet 20 r=AurelienFT a=AurelienFT

Merged in this testnet:

- #3475 
- #3549
- #3562 
- #3462 
- #3492 
- #3502 
- #3495 
- #3556 
- #3511 
- #3498 
- #3566 
- #3557 
- #3576 
- #3579 
- #3507 
- #3585 
- #3587 
- #3580 
- #3590 
- #3549 
- #3455 
- #3601 
- #3602 
- #3606 
- #3588 
- #3603 
- #3554

Co-authored-by: AurelienFT <[email protected]>
Co-authored-by: Ben PHL <[email protected]>
Co-authored-by: Modship <[email protected]>
Co-authored-by: Ben <[email protected]>
Co-authored-by: Eitu33 <[email protected]>
Co-authored-by: Sydhds <[email protected]>
Co-authored-by: modship <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
@Ben-PH Ben-PH mentioned this pull request Mar 20, 2023
28 tasks
/// Executed operations btreemap with slot as index for better pruning complexity
pub sorted_ops: BTreeMap<Slot, PreHashSet<OperationId>>,
/// Executed operations only for better insertion complexity
pub ops: PreHashSet<OperationId>,
Copy link
Member

@damip damip Mar 27, 2023

Choose a reason for hiding this comment

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

how about just changing this into a PreHashMap<OperationId, Result<(), String>> that will contain the outcome of an operation execution (and the error message if any), instead of having another container that we must not forget to update/clear ?

Comment on lines +270 to +276
pub fn get_op_exec_status(&self) -> HashMap<OperationId, bool> {
self.0
.iter()
.flat_map(|exec_output| exec_output.state_changes.executed_ops_changes.clone())
.map(|(op_id, (status, _))| (op_id, status))
.collect()
}
Copy link
Member

@damip damip Mar 27, 2023

Choose a reason for hiding this comment

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

    /// Check the outcome of the execution of an operation
    pub fn get_op_exec_status(&self, operation_id: &OperationId) -> HistorySearchResult<Result<(), String>> {
        for item in self.0.iter() {
            if let Some((outcome, _slot)) = item.state_changes.executed_ops_changes.get(operation_id) {
                return HistorySearchResult::Present(outcome.clone());
            }
        }
        HistorySearchResult::NoInfo
    }

Comment on lines +1413 to +1418
pub fn get_op_exec_status(&self) -> (HashMap<OperationId, bool>, HashMap<OperationId, bool>) {
(
self.active_history.read().get_op_exec_status(),
self.final_state.read().executed_ops.op_exec_status.clone(),
)
}
Copy link
Member

@damip damip Mar 27, 2023

Choose a reason for hiding this comment

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

    /// (final, active)
    pub fn get_op_exec_status(&self, operation_id: &OperationId) -> (Option<Result<(), String>>, Option<Result<(), String>>) {
        if let Some(outcome) = self.final_state.read().get_op_exec_status(operation_id) {
            return (Some(outcome.clone()), Some(outcome.clone()));
        }
        if let HistorySearchResult::Present(outcome) = self.active_history.read().get_op_exec_status(operation_id) {
            return (None, Some(outcome.clone()));
        }
        (None, None)
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ability to see if a SC execution succeeded

4 participants