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.

Sign up for GitHub

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

Jump to bottom

Migrate provided id test #21

Merged
merged 5 commits into from
Aug 5, 2023
Merged

Migrate provided id test #21

merged 5 commits into from
Aug 5, 2023

Conversation

v9n
Copy link
Member

@v9n v9n commented Jul 27, 2023 •
edited
Loading

We have updated our chain API in runtime 2.9.5 and oak-collator 2.0.0. it is a breaking changes so we updated this to reflect our task id change.

Before:

task id can be infer on client side using provided id

After:

client need to fetch events and find task id to assert or do more CRUD.

Test result run againt a local node on bump-to-2.0.0 branch on revision e3b6896bf6d6d9f07a4fdbe70155b8cccfe140f1

 PASS  test/functional/rpc.test.ts
  ✓ scheduler.getTimeAutomationFees works (144 ms)
  ✓ scheduler.calculateOptimalAutostaking works (104 ms)
  ✓ scheduler.getAutoCompoundDelegatedStakeTaskIds works (95 ms)

Test Suites: 3 passed, 3 total
Tests:       47 passed, 47 total
Snapshots:   0 total
Time:        682.918 s, estimated 7029 s

@v9n v9n marked this pull request as draft July 27, 2023 01:22
@v9n v9n force-pushed the update-test-for-new-taskid-refactor branch from d252c7e to f5c16f7 Compare July 29, 2023 00:35
@v9n v9n changed the title wip migrate provided id Migrate provided id test Jul 31, 2023
@v9n v9n marked this pull request as ready for review July 31, 2023 16:47
@v9n v9n requested review from chrisli30 and imstar15 July 31, 2023 16:47
@v9n v9n self-assigned this Jul 31, 2023
chrisli30
chrisli30 reviewed Jul 31, 2023
View reviewed changes
Copy link
Member

@chrisli30 chrisli30 left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, but I have got two comments.

test/functional/cancel.test.ts Show resolved Hide resolved
test/utils/helpFn.ts Show resolved Hide resolved
imstar15
imstar15 requested changes Aug 3, 2023
View reviewed changes
Copy link
Member

@imstar15 imstar15 left a comment

Choose a reason for hiding this comment

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

In this file, there are still some places that need to be modified.

https://github.com/OAK-Foundation/oak.js/blob/f5ef74c9246fc45a2fb265be528146a81bfa020c/packages/types/src/automationTime.ts#L3

chrisli30
chrisli30 previously approved these changes Aug 3, 2023
View reviewed changes
test/functional/cancel.test.ts Show resolved Hide resolved
test/functional/cancel.test.ts Show resolved Hide resolved
imstar15
imstar15 reviewed Aug 4, 2023
View reviewed changes
test/utils/helpFn.ts
@@ -154,7 +149,7 @@ export const cancelTaskAndVerify = async (automationTimeApi: AutomationTimeApi,

expect(section).toEqual(SECTION_NAME);
expect(method).toEqual('cancelTask');
expect(taskIdOnChain.toString()).toEqual(taskID);
expect(new Buffer(taskIdOnChain).toString()).toEqual(taskID);
Copy link
Member

Choose a reason for hiding this comment

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

Why is new Buffer(...) being used here?

imstar15
imstar15 approved these changes Aug 4, 2023
View reviewed changes
Copy link
Member

@imstar15 imstar15 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

imstar15
imstar15 requested changes Aug 4, 2023
View reviewed changes
Copy link
Member

@imstar15 imstar15 left a comment

Choose a reason for hiding this comment

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

/packages/types/src/xcmpHandler.ts is also needed to change.

https://github.com/OAK-Foundation/oak.js/blob/f5ef74c9246fc45a2fb265be528146a81bfa020c/packages/types/src/xcmpHandler.ts#L23

@v9n
Copy link
Member Author

v9n commented Aug 4, 2023

@imstar15 Ready to review again

@v9n
Copy link
Member Author

v9n commented Aug 4, 2023

@chrisli30 @imstar15 This PR is just to update the test. I will re-gen those type def and edgeware.json in separate PR.

chrisli30
chrisli30 previously approved these changes Aug 5, 2023
View reviewed changes
Copy link
Member

@chrisli30 chrisli30 left a comment

Choose a reason for hiding this comment

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

It’s good to go!

imstar15
imstar15 approved these changes Aug 5, 2023
View reviewed changes
Copy link
Member

@imstar15 imstar15 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@v9n v9n merged commit 1310576 into main Aug 5, 2023
1 check passed
@v9n v9n deleted the update-test-for-new-taskid-refactor branch August 5, 2023 02:46
@v9n v9n mentioned this pull request Aug 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imstar15 imstar15 imstar15 approved these changes

@chrisli30 chrisli30 chrisli30 left review comments

Assignees

@v9n v9n

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@v9n @chrisli30 @imstar15