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

Update system API for 128 bit cycles tests #57

Merged
merged 6 commits into from Nov 20, 2021
Merged

Conversation

AlexandraZapuc
Copy link
Contributor

Following up on our discussion regarding the multivalue target feature support, we have decided to update the experimental System Api and not rely on this feature anymore.

This MR updates the tests according to the Public Spec.

@nomeata
Copy link
Contributor

nomeata commented Nov 11, 2021

Thanks, that reduces my worries!

I wonder if its worth simplifying the universal canister interface, undo the addition of I128 to the interface, and use two I64 for the arguments, and Blob for the results. This way the universal canister does less and is closer to the system API. WDYT, @marcin-dziadus ?

@marcin-dziadus
Copy link
Contributor

Thanks, that reduces my worries!

I wonder if its worth simplifying the universal canister interface, undo the addition of I128 to the interface, and use two I64 for the arguments, and Blob for the results. This way the universal canister does less and is closer to the system API. WDYT, @marcin-dziadus ?

Sorry for the late response - I was on vacation for a few days.
@nomeata Yes, it's a very good point! I'll take care of it.

@marcin-dziadus
Copy link
Contributor

marcin-dziadus commented Nov 20, 2021

@nomeata I'm done with the refactoring, could you please take a look?

BTW, one of the checks is constantly failing. Do you perhaps know why?

@marcin-dziadus marcin-dziadus marked this pull request as ready for review November 20, 2021 09:51
src/IC/Canister/Imp.hs Outdated Show resolved Hide resolved
src/IC/Canister/Imp.hs Outdated Show resolved Hide resolved
@nomeata
Copy link
Contributor

nomeata commented Nov 20, 2021

About the failing check: A nne nix release nieeds an optio to be set; have a look at dfinity/motoko#2880 for how to fix this.

@marcin-dziadus
Copy link
Contributor

marcin-dziadus commented Nov 20, 2021

About the failing check: A nne nix release nieeds an optio to be set; have a look at dfinity/motoko#2880 for how to fix this.

Thanks a lot! (for this, as well as the review)

@marcin-dziadus marcin-dziadus merged commit 728d9da into master Nov 20, 2021
@marcin-dziadus marcin-dziadus deleted the exc-649 branch November 20, 2021 14:17
@nomeata
Copy link
Contributor

nomeata commented Feb 6, 2022

@marcin-dziadus, guess we missed the chance to use ic-ref-test to catch bugs in production: It doesn't actually test if cycle balances above 2^64 work, right? If we did, this might have prevented https://forum.dfinity.org/t/panicked-at-failed-to-serialize-message-errorimpl-code-message-the-number-cant-be-stored-in-cbor-offset-0-build-rs-canister-sandbox-common-src-transport-rs55/10729?u=nomeata…

@nomeata
Copy link
Contributor

nomeata commented Feb 6, 2022

Hmm, nevermind, there is a test here:

          let large = 2^(65::Int)
          ic_top_up ic00 cid large

so now I am curious how that relates to the bug I reported at the forum.

Is ic-ref-test still run against the replica as part of CI?

@marcin-dziadus
Copy link
Contributor

marcin-dziadus commented Feb 7, 2022

Yes, it is, but we haven't released a new version of ic-ref(-test) for a while.
The 128 bit API is not included in v18.0 which is currently run on a regular basis against the replica.

@nomeata
Copy link
Contributor

nomeata commented Feb 7, 2022

So the replica ships with features that don't have an spec acceptance test in CI yet? I guess we can't have everything

@marcin-dziadus
Copy link
Contributor

We should optimise the process. I will raise this issue.

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.

None yet

3 participants