-
Notifications
You must be signed in to change notification settings - Fork 480
Handle LangError from instantiate
#1512
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
90 commits
Select commit
Hold shift + click to select a range
8796a62
Handle `LangError` from instantiate (fails for success case)
HCastano 95b80c4
Change generic in `CreateBuilder` to be more consistent
HCastano a49fcb7
Remove extra generic parameter
HCastano 8312fa8
Remove generic return type parameter from `CreateBuidler` codegen
HCastano 6e70be0
Hardcode assumption that `instantiate` returns a `ConstructorResult`
HCastano 2fd4a82
Update `CreateBuilder` codegen to just return `Self`
HCastano 4d1f2bf
Remove generic usage to fix formatting
HCastano f107cb1
Unwrap `ConstructorResult` in `contract-ref` E2E test
HCastano b6de9fc
Clean up some comments
HCastano 5d221c3
Bring back the assumption that we expect an `AccountId`
HCastano 7d94f75
Remove unused method
HCastano 798f946
Update doc tests for new builder pattern
HCastano 18d0922
Clean up some comments
HCastano 06bba81
Fix Clippy warning
HCastano 612537e
Fix typo
HCastano 2175174
Add `try_instantiate` method to `CreateBuilder`
HCastano 44a8324
Remove unneeded `unwrap`
HCastano 84d257d
Remove debug logging
HCastano ffcca2c
Update doc test
HCastano 91cd98f
Fix some typos
HCastano e701230
Mention panicking behaviour of `instantiate` methods
HCastano f2d85dd
Improve error messages from wrong `returns()` type
HCastano addcda2
Actually check return values from `call_instantiate`
HCastano 4c99daf
Add test showing a reverting constructor with `Ok` in error buffer
HCastano 99c859b
Check that we're only returning `LangError`s if the contract reverted
HCastano 323ed2d
Clean up the manual encoding test a bit
HCastano b6fc625
Add test for constructors which return a contract level error
HCastano ac6b197
Add `CreateBuilder` message which calls a fallible constructor
HCastano b06a9f8
Add test which calls falliable constructor for success case
HCastano 7d664a0
Get verbose `instantiate_contract_with_result` decoding past typechecker
HCastano f874c97
Add `try_instantiate_with_result` to `CreateBuilder`
HCastano c29895a
Clean up decoding logic for output from `seal_instatiate`
HCastano dbb079e
Small cleanups in `call-builder` E2E tests
HCastano 6125690
RustFmt `env_access`
HCastano aa4f897
Remove unused import
HCastano 053218f
Flip decoding logic so that it's more strict initially
HCastano 77c790f
Add test which revert a fallible constructor
HCastano 713ad1e
Remove note about removing `assert` statement
HCastano 64a711c
Check return value from fallible constructor tests
HCastano 54fcf90
Merge branch 'master' into hc-get-lang-error-from-create-builder
HCastano c17fc3b
Update E2E Builder typedef to match changes
HCastano 32f4a75
Update E2E test for new call syntax
HCastano a3cfbb8
Use `selector_bytes!` macro in more places
HCastano 283e182
Change order to accounts used in tests
HCastano cc98baf
Update function names to use `fallible`
HCastano f01b5ca
Add note about docs
HCastano c794ec9
Merge branch 'master' into hc-get-lang-error-from-create-builder
HCastano b4290cb
Update `ContractRef` codegen to use fallible constructor return types
HCastano d47bf8f
Stop returning an `AccountId` directly from `CreateBuilder::try_insta…
HCastano b720ede
Add panicking version of `try_instantiate_fallible`
HCastano 75cad9e
Add test for using fallible constructors through ContractRefs
HCastano cf2516f
Add test for instantiation failure too
HCastano f9b3004
Add `instantiate_fallible` to `CreateParams`
HCastano d511b3f
Add a couple of missing docs
HCastano 550b728
Convert `call-builder` test return type to `AccountId`
HCastano 1668ae0
Extract reverted fallible constructor fn for testing
ascjones acf23b9
Fmt
ascjones e69e7ce
Add fallible_constructor_reverted_lang_error FAILs
ascjones f084763
Rename tests
ascjones 74f2392
Add test for a decode error
ascjones e3a99f9
Rename some tests
ascjones 56e8d52
Make `Result` types more explicit
HCastano 50c227b
Add another test
HCastano 8ce1d92
Clean up decoding match statement
HCastano c291b3c
Andrew was right
HCastano f0561e5
Small cleanups to naming and imports
HCastano 1742a01
Couple more import and comment fixes
HCastano 25d8326
Use decode trait method directly
HCastano 24e9424
Fix `call-builder` E2E tests
HCastano e7bba67
Remove leading colons from non-codegen contexts
HCastano ebd42f3
Add doc test for `instantiate_fallible_contract`
HCastano 0a99f16
Add doc test to `build_create` function
HCastano 6cf60e6
Remove leftover trait bound
HCastano 581291b
Remove a few more leading colons
HCastano 95f9e6e
Panic in case where we get `Ok` encoded into error buffer
HCastano 0d40825
Add some links to env docs
HCastano c8e5cfa
Add more docs to `call-builder` E2E tests
HCastano 1e323f4
Use correct path in `call-builder` docs
HCastano 0b31392
Merge branch 'master' into hc-get-lang-error-from-create-builder
ascjones 9a14e21
Use return_value() method in e2e test
ascjones 69946de
Merge branch 'master' into hc-get-lang-error-from-create-builder
HCastano 37d8e80
Fix `call-builder` E2E test compilation
HCastano 0c568b5
Fix `contract-ref` E2E test compilation
HCastano 7d20051
Fix some of the comment links
HCastano eeb7b85
Unwrap errors from default `instantiate_fallible` codepath
HCastano 658fed1
Fix `contract-ref` E2E test
HCastano aac62db
Wrap long line
HCastano 619cc71
Remove TODO
HCastano dd41485
Fix instatiation doc test
HCastano b41ca87
Merge branch 'master' into hc-get-lang-error-from-create-builder
HCastano File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Clean up some comments
- Loading branch information
commit b6de9fc968fce60e077f3195ff1b045ac62fc424
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Couldn't there exist a contract that would revert the contract but return
Ok(())e.g.This could mislead the caller into handling the
Okcase?Uh oh!
There was an error while loading. Please reload this page.
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.
Hmm yeah this is a good observation. I've added a test that shows that you can indeed revert from a constructor and continue execution by returning
Ok(AccountId).I've also added a quick patch which asserts that the content in the buffer is an error.
I'm not entirely sure what the implications of either case are though...
Revert + Okcase, misleading the caller seems like the main reason to do this, but maybe there are legit reasons to do encode anOkback? I can't think of any though.Revert + Errcase, the callee can encode the "wrong"LangErrorinto the output buffer, potentially misleading the caller too, and there's nothing we can do to help there.Either way it'll be up to the contract developer to make sure they understand that there are risks to calling other contracts.
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.
@athei would like to hear your opinion here
Uh oh!
There was an error while loading. Please reload this page.
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.
The assumption is that the user is not calling
return_valuedirectly, and if they are they are using it in an ABI compatible way which is eitherRevert + Err(LangError)orRevert + Ok(Err(ContracErr)).So perhaps we could return an
Err(LangError)here if this ABI is not satisfied, e.g. as mentioned in #1512 (comment)Uh oh!
There was an error while loading. Please reload this page.
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.
Okay maybe what we want is something along the lines of