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

Simplify errors #36

Merged
merged 11 commits into from
Mar 13, 2022
Merged

Simplify errors #36

merged 11 commits into from
Mar 13, 2022

Conversation

stechu
Copy link
Collaborator

@stechu stechu commented Mar 10, 2022

Get rid of stand alone LedgerInternalError

Signed-off-by: Shumo Chu <shumo.chu@protonmail.com>
Signed-off-by: Shumo Chu <shumo.chu@protonmail.com>
Signed-off-by: Shumo Chu <shumo.chu@protonmail.com>
Signed-off-by: Shumo Chu <shumo.chu@protonmail.com>
Signed-off-by: Shumo Chu <shumo.chu@protonmail.com>
Signed-off-by: Shumo Chu <shumo.chu@protonmail.com>
@stechu stechu requested a review from bhgomes March 10, 2022 08:29
Copy link
Contributor

@bhgomes bhgomes left a comment

Choose a reason for hiding this comment

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

What I meant for structuring the errors was something like the following:

trait TransferLedger {
    /// State Update Error
    type UpdateError;

    fn update_public_balances(...) -> Result<(), Self::UpdateError>;
}

where this error is core::convert::Infallible for the mock ledger and for substrate it can be some other error.

Also, please remove the dead code introduced by the previous PR, namely the AccountBalance enum and the LedgerInternalError struct.

Copy link
Contributor

@bhgomes bhgomes left a comment

Choose a reason for hiding this comment

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

We basically want the ledger to state whether updating the public balances can fail or not. In the case of the mock ledger, it cannot so the error type is Infallible but for substrate, we may have some more complex requirements. Eventually, we want to revisit the error states of the ledgers since they assume no errors during other state updates but we may want to make this explicit. Currently, the API is a bit asymmetric because only update_public_balances gets an error condition, but this is fine for now.

manta-accounting/src/transfer/mod.rs Outdated Show resolved Hide resolved
manta-accounting/src/transfer/mod.rs Outdated Show resolved Hide resolved
manta-accounting/src/transfer/mod.rs Outdated Show resolved Hide resolved
manta-pay/src/simulation/ledger/mod.rs Outdated Show resolved Hide resolved
manta-accounting/src/transfer/mod.rs Show resolved Hide resolved
Signed-off-by: Shumo Chu <shumo.chu@protonmail.com>
Signed-off-by: Shumo Chu <shumo.chu@protonmail.com>
@stechu stechu merged commit dc68e8d into main Mar 13, 2022
@stechu stechu deleted the simplify-errors branch March 13, 2022 22:56
tsunrise pushed a commit that referenced this pull request Mar 18, 2022
* refactor to adapt manta-pay integration

Signed-off-by: Shumo Chu <shumo.chu@protonmail.com>
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.

2 participants