Error handling, extra fields, and error handling for IDL deserialization #53

chenyan-dfinity · 2019-10-03T19:27:30Z

Support extra fields for struct/enum
Bug fix for deserializing nested struct/enum
Remove unwrap in the deserialization code and dump error state for debugging.
More unit tests

hansl

Early review. Thanks for the PR, this looks better already.

hansl · 2019-10-07T21:44:09Z

lib/serde_idl/src/de.rs

+            .de
+            .types
+            .pop_front()
+            .ok_or_else(|| Error::msg("No more values to deserialize"))?;


You used to have a Eof error enum value. Is it not appropriate here?

I don't think enum is helpful for error message. I just refactor the Error module to be a struct that contains just a string message, and error states. Specifically for Eof, we would like to know if the Eof is due to asking for more values to deserialize, or Eof during deserializing a value.

Most of those can be used when testing too. Adding error types add a lot of information about what's going on.

Can you give an example where adding error types can be useful? If I add an error type Eof here, I don't see how it is going to be more useful for either the user or the library developer, given that we already have the specific message string for each error.

hansl · 2019-10-07T21:55:14Z

lib/serde_idl/src/de.rs

-                "Trailing types {:?}, field_index: {:?}",
-                self.de.current_type, self.de.field_index
+            Err(Error::msg(format!(
+                "Trailing types {:?}, field_name {:?}",


We probably should make most of those into separate types so we can sort them.

What do you mean by "sort"?

Not lexicographic sort. I'm just asking for error types for everything (and not just a msg(String) error type).

lib/serde_idl/src/de.rs

lib/serde_idl/src/lib.rs

hansl · 2019-10-07T22:06:51Z

lib/serde_idl/src/de.rs

+                            }
+                            None => {
+                                if !fs.is_empty() {
+                                    return Err(Error::msg(format!(


Let's add custom error enumeration values for all different error types.

I think we need concrete error messages, not enum. If we want to categorize the message, it would all be "deserialize error"

hansl · 2019-10-07T22:07:59Z

lib/serde_idl/src/de.rs

                for i in 0..len {
-                    let hash = self.de.current_type.pop_front().unwrap().get_u64()? as u32;
-                    let ty = self.de.current_type.pop_front().unwrap();
+                    let hash = self.de.pop_current_type()?.get_u64()? as u32;


I understand that we hash to 32 bits, but is there a reason we encode/decode to 64? Is it because we don't have a leb32()?

Yes, leb128 crate decodes to 64. We probably need to write our own leb128 module to support bignum. So I wouldn't worry about the type conversion for now.

eftychis · 2019-10-08T20:26:02Z

lib/serde_idl/src/de.rs

+    }
+
+    fn dump_error_state(&self, e: Error) -> Error {
+        /*


? reason for commenting?

eprintln will print out the states which make e2e unhappy. I just add the states into the Error struct.

We shouldn't dump the state, but let the caller show the error message if there's anything.

Remember this is a library we (and probably many others) will use outside of dfx.

Caller has no way to dump the state even if wanted, since the states are internal. I am okay with having two version of Encode/Decode, where the public one has no state dump, and a debug version that has the states. FYI, dumping states have been tremendously helpful for me to debug bugs in the deserialization. It would be also helpfully for people to send bug reports. If the caller doesn't want the states, they can always remove the states via map_err in their code.

lib/serde_idl/src/de.rs

eftychis · 2019-10-09T22:22:29Z

lib/serde_idl/src/lib.rs

-// IDL hash function is specified in
-// https://github.com/dfinity-lab/actorscript/blob/master/design/IDL.md#shorthand-symbolic-field-ids
-// which comes from https://caml.inria.fr/pub/papers/garrigue-polymorphic_variants-ml98.pdf
+// IDL hash function comes from


As this is a public function I would add a bit more documentation.

I don't think we need idl_hash anywhere else than here, so maybe make it pub(crate)?

Moved idl_hash to dfx_info crate, also move the idltype.rs test to dfx_info.

lib/serde_idl/tests/serde.rs

eftychis

LGTM Yan :)

P.S. Just fix the couple of comments above.

resolved

eftychis · 2019-10-14T16:42:47Z

@stanleygjones @paulyoung @chenyan-dfinity This is stuck. I can't override it, but github does not want this merged yet...

paulyoung · 2019-10-14T16:52:57Z

Mergify says: "Rule: Automatic merge (squash) (merge) In progress — Waiting for the Branch Protection to be validated"

Whatever that means.

paulyoung · 2019-10-14T16:54:24Z

Everything seems to have passed and I can merge manually without using admin privileges so that's what I'm going to do.

eftychis · 2019-10-14T16:56:48Z

Hm it didn't let me merge -- are you sure it didn't want admin?

paulyoung · 2019-10-14T16:58:15Z

I'm sure. It definitely didn't show the "you can still use admin privileges to merge" message.

chenyan-dfinity · 2019-10-14T17:30:23Z

Thanks for the merging!

@hansl

commit 19c28008db5fa1582da573088b17d0bdee9e8bc2 Author: Hans <hans@larsen.online> Date: Mon Nov 9 14:47:25 2020 -0800 revert: Remove lerna (PR #47) (#59) This removes lerna, but attempt to not remove the e2e tests and the new packages/files that were added after the last PR. commit 426a94f356bc4e27f50c8bec55071b82105adbd1 Author: Hans <hans@larsen.online> Date: Mon Nov 9 13:54:04 2020 -0800 feat: use anonymous for the first retrieve (#53) Also include some cleanup of the scripts that arent used, and add a diagram for the bootstrapping process. This does not affect the worker yet. Only the first retrieve of the index.js on localhost. BREAKING CHANGE If a canister has a retrieve() method that relies on Principal, the principal used will change. Co-authored-by: Benjamin Goering <benjamin.goering@dfinity.org> commit b795d8b34ce38dd35e1bcf2b80e33108cf74161d Author: Andrew Wylde <2126677+codelemur@users.noreply.github.com> Date: Mon Nov 9 09:41:13 2020 -0800 feat(idp): add identity-provider package to repository (#52) adds plain HTML/ts and test setup for project commit 7c5e71488df430b89de637a15b56814db196e752 Author: Islam El-Ashi <ielashi@users.noreply.github.com> Date: Sat Nov 7 00:30:05 2020 +0100 feat: DER-encode public key and principal ID when using Plain Authentication. (#48) BREAKING CHANGE If a developer relies on the inner work of SenderPubKey or manually instanciate Principal, the API changed. commit a254861ae4bfe608734c8c190e4033cfe39f047b Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:32 2020 -0800 chore(deps): bump node-fetch from 2.6.0 to 2.6.1 in /e2e/node (#49) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 694849109bc28fea80119388504127e889b36d15 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:14 2020 -0800 chore(deps-dev): bump node-fetch from 2.6.0 to 2.6.1 in /packages/agent (#50) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 88af47f79b5e9f3a5e08abbf0ce03b3140357fc0 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:16:53 2020 -0800 chore(deps-dev): bump node-fetch in /packages/bootstrap (#51) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit ba4b082e5795150176f7adad989c3613edceb85f Author: Hans <hans@larsen.online> Date: Thu Nov 5 12:49:59 2020 -0800 test: add ic-ref, ci e2e and lerna support to repo (#47) This refactors the repository to use lerna to manage its monorepo's packages. It also adds the e2e tests from the SDK repo that we previously had to remove. More e2e tests will be added later, but this unblocks working with DER and Identity work as we can now at least run some e2e tests now (instead of mocking the http connection). commit 8baa1cfc227d003c2806df1057b92009159df250 Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:36 2020 -0800 feat: use anonymous principal by default in HttpAgent (#46) If the auth transform is missing and the principal is anonymous, use an anonymous auth transform. Otherwise, use the auth transform as normal. # BREAKING CHANGE Before, an auth transform would not be called if it was missing, now it results as an error if the principal is not anonymous. This was because just using the packet as is without putting it in an envelope was a bug and should never be sent to a replica anyway. Now this is explicit. commit 1a714e6c53f5e467112b964b71204b6d2eb545ef Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:15 2020 -0800 feat: add a getPrincipal() method to Agents (#40) commit 324e856944adb7f79a2afa2cffbdb6d144b35ab4 Author: Hans <hans@larsen.online> Date: Tue Nov 3 08:49:44 2020 -0800 feat: add the canister ID to the window.ic object (#41) This can be useful to know the canister ID of the current frontend. commit 8680a2cf4994b45c6e00ffb0b5a12b379f5ff919 Author: Hans <hans@larsen.online> Date: Mon Nov 2 21:57:21 2020 -0800 feat: add a cache bust hash at the end of javascript output (#39) commit 9f2ed968b7cc9f4b854af5e23edca19d34124909 Author: Benjamin Goering <benjamin.goering@dfinity.org> Date: Tue Sep 22 15:28:49 2020 -0700 docs: polish docs/npm-publish.md per @hansl feedback (#37) via: pr feedback on dfinity/agent-js#36

chenyan-dfinity added 12 commits October 3, 2019 12:26

refactor and check_error

8829b6d

checkpoint for struct subtype

0938a33

checkpoint

8e0e02a

remove unwrap

326fc15

checkpoint

e75cad2

struct subtype works

20f5d30

enum inside struct works

96089ef

cargo fmt

ce4a458

cargo fmt

314f028

update doc

ac8ea2a

fix

5f11452

fix

9e8e1a5

chenyan-dfinity changed the title ~~Error handling and tests for deserialization~~ Error handling, extra fields, and error handling for IDL deserialization Oct 7, 2019

chenyan-dfinity requested review from hansl, eftychis and matthewhammer October 7, 2019 21:42

chenyan-dfinity marked this pull request as ready for review October 7, 2019 21:42

chenyan-dfinity requested a review from a team as a code owner October 7, 2019 21:42

Merge branch 'master' into idl

f802e9d

hansl previously requested changes Oct 7, 2019

View reviewed changes

chenyan-dfinity and others added 3 commits October 8, 2019 10:39

tests

6463e1a

Merge branch 'master' into idl

a65a6b4

remove eprintln to pass e2e

e601aec

eftychis reviewed Oct 8, 2019

View reviewed changes

chenyan-dfinity added 3 commits October 8, 2019 15:27

add states to error

470a8ab

fix

4d11534

out-of-bounds check

289c6d2

eftychis reviewed Oct 9, 2019

View reviewed changes

lib/serde_idl/src/de.rs Show resolved Hide resolved

make error module more generic to take strings

52bfba8

eftychis reviewed Oct 9, 2019

View reviewed changes

lib/serde_idl/tests/serde.rs Show resolved Hide resolved

eftychis approved these changes Oct 9, 2019

View reviewed changes

chenyan-dfinity and others added 4 commits October 10, 2019 12:28

move idltype test to dfx_info; add docs for public functions

73351c7

fix

850ce39

fix doc

ac67ac4

Merge branch 'master' into idl

736f7d4

chenyan-dfinity mentioned this pull request Oct 11, 2019

IDL in Rust: Some additional tests #51

Closed

chenyan-dfinity requested a review from hansl October 11, 2019 20:49

Merge branch 'master' into idl

426561e

paulyoung added the automerge-squash label Oct 11, 2019

paulyoung merged commit 2be3988 into master Oct 14, 2019

mergify bot deleted the idl branch October 14, 2019 16:55

mergify bot removed the automerge-squash label Oct 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error handling, extra fields, and error handling for IDL deserialization #53

Error handling, extra fields, and error handling for IDL deserialization #53

chenyan-dfinity commented Oct 3, 2019 •

edited

Loading

hansl left a comment

hansl Oct 7, 2019

chenyan-dfinity Oct 8, 2019

hansl Oct 9, 2019

chenyan-dfinity Oct 9, 2019

hansl Oct 7, 2019

chenyan-dfinity Oct 8, 2019

hansl Oct 9, 2019

hansl Oct 7, 2019

chenyan-dfinity Oct 8, 2019

hansl Oct 7, 2019

chenyan-dfinity Oct 8, 2019

eftychis Oct 8, 2019

chenyan-dfinity Oct 8, 2019

hansl Oct 9, 2019

chenyan-dfinity Oct 9, 2019

eftychis Oct 9, 2019

hansl Oct 9, 2019

chenyan-dfinity Oct 10, 2019

eftychis left a comment •

edited

Loading

eftychis commented Oct 14, 2019

paulyoung commented Oct 14, 2019

paulyoung commented Oct 14, 2019

eftychis commented Oct 14, 2019

paulyoung commented Oct 14, 2019

chenyan-dfinity commented Oct 14, 2019

Error handling, extra fields, and error handling for IDL deserialization #53

Error handling, extra fields, and error handling for IDL deserialization #53

Conversation

chenyan-dfinity commented Oct 3, 2019 • edited Loading

hansl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eftychis left a comment • edited Loading

Choose a reason for hiding this comment

eftychis commented Oct 14, 2019

paulyoung commented Oct 14, 2019

paulyoung commented Oct 14, 2019

eftychis commented Oct 14, 2019

paulyoung commented Oct 14, 2019

chenyan-dfinity commented Oct 14, 2019

chenyan-dfinity commented Oct 3, 2019 •

edited

Loading

eftychis left a comment •

edited

Loading