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

fix(xs-worker): respect !managerOptions.metered #3078

Merged
merged 3 commits into from
May 17, 2021
Merged

Conversation

dckc
Copy link
Member

@dckc dckc commented May 11, 2021

goal: fixes #3074

This doesn't have an automated test for the main change.

Ideally I'd thread an instrumented version of spawn that records the args for the test to check. But spawn is imported from ambient authority in controller.js rather than being passed explicitly, so it's not obvious how to do this.

@dckc
Copy link
Member Author

dckc commented May 11, 2021

unrelated test failure in ci; for follow-up, see #3079

  message: 'newSwap doubleSwap threw: cannot coerce object to string',
  filename: 'test/unitTests/contracts/newSwap/test-newSwap-swap.js',

1 tests failed
F test/unitTests/contracts/newSwap/test-newSwap-swap.js newSwap doubleSwap

reproduced with

  • yarn test:xs-unit test/unitTests/contracts/newSwap/test-newSwap-swap.js

failed to reproduce with

  • yarn test:xs-unit test/unitTests/contracts/newSwap/test-newSwap-swap.js -m 'newSwap doubleSwap'
  • yarn test:xs-unit --debug test/unitTests/contracts/newSwap/test-newSwap-swap.js

@dckc
Copy link
Member Author

dckc commented May 11, 2021

b646e7d tests that we can increment the counter and just not compare it against a threshold

@dckc dckc marked this pull request as ready for review May 11, 2021 19:51
@dckc dckc changed the title fix(xs-worker): respect metered from managerOptions fix(xs-worker): respect !managerOptions.metered May 12, 2021
@dckc dckc force-pushed the 3074-no-meter-sys branch 2 times, most recently from 7d39919 to 14c7393 Compare May 14, 2021 19:15
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

Yeah, it'd be nice to have that extra test, but it's not a big deal.

Hm, how messy would it be to build startXSnap with a spawn that recorded the arguments in a retrievable side-something and then called the real spawn? Probably too messy.

@dckc dckc force-pushed the 3074-no-meter-sys branch from 14c7393 to c885d23 Compare May 17, 2021 14:55
@dckc
Copy link
Member Author

dckc commented May 17, 2021

... how messy would it be to build startXSnap with a spawn that recorded the arguments in a retrievable side-something and then called the real spawn? Probably too messy.

On the contrary... a bit more ocap discipline doesn't look messy at all, to me. :) c885d23

@dckc dckc requested a review from warner May 17, 2021 15:14
@dckc dckc force-pushed the 3074-no-meter-sys branch from c885d23 to 97b90a0 Compare May 17, 2021 16:23
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

nice! hooray for ocaps

@dckc dckc merged commit 84fa8c9 into master May 17, 2021
@dckc dckc deleted the 3074-no-meter-sys branch May 17, 2021 16:57
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.

"system" vats should not be limited by the compute meter
2 participants