Scheduled callbacks #823
Scheduled callbacks #823
Conversation
devbugging
requested review from
janezpodhostnik,
bluesign,
chasefleming,
jribbink and
peterargue
as code owners
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@chasefleming added changes. |
Co-authored-by: Bastian Müller <[email protected]>
| return 0, nil, fmt.Errorf("invalid event type, got: %s, expected: %s", event.Type, callbackScheduledEvent) | ||
| } | ||
|
|
||
| id, ok := event.Value.SearchFieldByName(IDField).(cadence.UInt64) |
There was a problem hiding this comment.
Nit: Instead of searching linearly for two fields, maybe use FieldsMappedByName instead and look up the fields in the result
|
From another chat, it sounds like we need to upgrade the contract to the latest version. |
|
I just pushed changes to update to the latest version of the contract and updated the tests as well. There will be future versions of the contract with significant changes, but this at least contains the basic functionality. |
|
@joshuahannan I'm not sure updating to real contract is a good idea here. This was supposed to be an early experimental verison of this feature not a production one. Since that contract is not yet done and tested we don't know how it will work with this integration, whereas this test contract I made I've tested it and it works. Unless you can do (for now manual) testing with the emulator to confirm it really works I wouldn't do this as part of this PR. |
|
@devbugging Can you elaborate more on why you think it might be an issue? The current version of the contract has all the most important functionality and the same API as the old one and has a decent number of unit tests already. I'm not sure why we'd want to use one that doesn't have full functionality. Maybe it isn't clear to me what the differences are that you're talking about |
|
@joshuahannan the only reason is because I'm sure the current version works as it should to use it. If you can make sure this updated contract also works as it should, then go for it. But don't think that current tests in emulator are enough to validate it works. This is because I wasn't writing e2e tests for this as it was experimental feature and e2e tests are being done in flow-go for final version. I did quite a bit of manual tests to make sure it works. So if you did as well then I have no objections. If this is updated and not really validated it works as it should then we can't merge it in. |
|
@devbugging Are the unit tests in flow-core-contracts enough to validate that the contract works as it should? So far, it seems to be working properly for the happy path for sure and definitely some edge cases too. afaik, this emulator release will just be used for internal testing, right? So I think it is okay to maybe take a small risk that something in the contract doesn't fully work if it means that we get full functionality of the contract and the correct events, API, etc for our internal testing |
|
@joshuahannan the contract tests make sure the contract works but doesn't check this integration. I'm fine if we use the new contract, but then we need to update to also use correct templates from flow-core-contract as well as the contract imported from there. So we either get in what we have and it's tested and make another PR that updates everything, or we update the emulator to use flow-core-contracts as it should and we must at least once run happy-path manually. I don't feel confident merging something noone tested. Either way it's fine with me, but I don't have now time to update the emulator to use flow-core-contracts. Maybe next week or so. If you have then that's great. Otherwise that was my concern with delays. |
|
Okay, I can spend some time this week updating it and testing it more |
|
@joshuahannan @devbugging - can I merge this now? Hoping to release to the CLI asap :) |
|
@jribbink Gregor told me that there were some issues when he was manually testing it so I don't know if it will work. I didn't understand the issues though. You might need to reach out to him to see what needs to be done because I don't think he gets notifications from github comments |
|
@jribbink I don't think this PR is currently in working condition. So we either revert back to my last commit which was a working experiemntal version since 2 months ago, or we update the emulator to work with current version, but I don't know when I will be able to spend time on updating emulator since my time is now focused on making flow-go work. |
|
Gotcha, I can wait until we have something working. I would agree that it doesn't make sense to ship an old API in this case, especially if there are significant changes and an update is already in the pipeline, as we'll just be repeating the same effort twice for little added benefit. Thank you! |
|
I'll spend some time this week to try to get it working |
|
@jribbink I got everything passing with the tests and the manual testing, so this is good to merge and make a release! Let me know if there is anything else you need for it |
Description
This is scheduled callback implementation on emulator.
It's experimental feature and contains unsafe implementation of callback scheduler contract, which simplifies the scheduling logic (and doesn't match how it will be on the mainnet). This is intended for early access only. The APIs should stay the same and behaviour of the APIs should stay similar (but there's no guarantee).
Contract is also added directly into project as well as transactions which will be refactored once this becomes part of core-contracts.
For contributor use:
masterbranchFiles changedin the GitHub PR explorer