-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix the usage of CallBuilder
pattern.
#10
Conversation
contracts/bulletin_board/lib.rs
Outdated
// Since the introduction of `ink::LangError` we got a new | ||
// "channel" where the contract errors may be pushed into. | ||
// In this particular example we have three layers of `Result` | ||
// types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: This is a bit confusing, what's this "channel" we're talking about? Why is it in quotes? Maybe make the comment more literal by saying something like: "the call return is wrapped in 3 levels of Result - the potential error related to making the call, a potential ink::LangError, and finally a potential HighlightedPostsError that stems from the return type of the method".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will simplify the comment and remove ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rephrased in a44181b
(#10)
contracts/bulletin_board/lib.rs
Outdated
match call_result { | ||
Err(ink_env_error) => { | ||
return Err(BulletinBoardError::from(ink_env_error)) | ||
} | ||
Ok(Err(lang_error)) => { | ||
panic!("Unexpected ink::LangError: {:?}", lang_error) | ||
} | ||
Ok(Ok(Err(contract_call_error))) => { | ||
return Err(BulletinBoardError::from( | ||
contract_call_error, | ||
)) | ||
} | ||
Ok(Ok(Ok(_unit))) => return Ok(()), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Not sure about adding this complicated match, are you trying to illustrate something? How about just fire()???
+ return Ok(())
? This is example code after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the match here is supposed to be an explanation of what kind of errors are being returned because it's not obvious, especially that one type of error (InkLangError
) is hidden from the contract authors.
This is example code after all.
Yes, this is an example but its main goal is maybe not how to write best contract code but show various elements of the contract and common pitfalls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right... In that case, maybe do an anyhow!("Some description of the kind of error in the given match arm")
for each of these cases? That way the code will be self-explanatory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a contract though, so I try to return some kind of error that can be (de)serialized. Only in the case of lang_error
I'm panicing as there's no way to handle that IIUC. Here there's some work where LangError
will, by default, not be handled and panic as well.
Respective entry in the migration guide has been added - https://www.notion.so/cardinalcryptography/Contracts-e9fcab6bc8094707ade4d243ea69994a#fb776d2f54ba4099acb8a04c185d7bca