Skip to content

Conversation

@kziemianek
Copy link
Contributor

@kziemianek kziemianek commented Nov 20, 2023

Makes fn perform_trusted_operation generic over return type so scalec decoding is handled internally.

We added it in Litentry worker in this PR and decided to propose it to the upstream repo 🙃 .

Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Nice, I believe this is a good ergonomic improvement! 👍 I have one question, otherwise it looks good!

@clangenb clangenb added A1-cli Affects cli functionalites B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E5-publicapi PR breaks public API labels Nov 21, 2023
@kziemianek kziemianek requested a review from clangenb November 21, 2023 08:07
Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Looks good to me now!

@kziemianek
Copy link
Contributor Author

@clangenb I think I may need help with solving this CI failure, I've tried to reproduce it locally with localy built binaries, started integritee node and worker in teeracle mode but the test pass :

./demo_teeracle_whitelist.sh -p 9944 -P 3000  -d 7 -i 10
Using client binary ./../bin/integritee-cli
integritee-cli 0.12.0
Using node uri ws://127.0.0.1:9944
Using trusted-worker uri wss://127.0.0.1:3000
Using worker market data update interval 10
Count the update events for 7 blocks

Minimum expected number of events with a single oracle source: 1
Minimum expected number of events with two oracle sources: 2
* Query on-chain enclave registry:
number of enclaves registered: 1
Enclave
   signer: AnySigner::Known(MultiSigner::Ed25519(fe2de3bfefabd2ab31e4c444518707254dd61ef067aa8fc4d47686e343d8ca4c (5HoyemQY...)))
   MRENCLAVE: 8ozuUjxfHoFYDWJhqNzuQhMUirWKzDSUH4KF3ECmfPoC
   RA timestamp: 1700642292001
   URL: wss://0.0.0.0:3000

Reading MRENCLAVE from worker list: 8ozuUjxfHoFYDWJhqNzuQhMUirWKzDSUH4KF3ECmfPoC

Listen to ExchangeRateUpdated events for 7 blocks. There should be no trusted oracle source!
Got 0 exchange rate updates when no trusted oracle source is in the whitelist

Add https://api.coingecko.com/ for 8ozuUjxfHoFYDWJhqNzuQhMUirWKzDSUH4KF3ECmfPoC as trusted oracle source
[+] Add to whitelist got finalized. Hash: 0xcd68db9962f03e61f56f0bd91fdbaab13470b22c19488622784fe83e5b869755

MRENCLAVE in whitelist for https://api.coingecko.com/

Listen to ExchangeRateUpdated events for 7 blocks, after a trusted oracle source has been added to the whitelist.
Got 2 exchange rate updates from the trusted oracle source in 7 blocks(s)

Add https://pro-api.coinmarketcap.com/ for 8ozuUjxfHoFYDWJhqNzuQhMUirWKzDSUH4KF3ECmfPoC as trusted oracle source
[+] Add to whitelist got finalized. Hash: 0xeb5b42d2bbf2f1f6c21703df6dd8d27a85defdebb4bd4190ee2362c2320543a0

MRENCLAVE in whitelist for https://pro-api.coinmarketcap.com/

Listen to ExchangeRateUpdated events for 7 blocks, after a second trusted oracle source has been added to the whitelist.
Got 4 exchange rate updates from 2 trusted oracle sources in 7 blocks(s)

Results :
test passed

Startup commands:

Starting integritee-node-process in background
Run node 1 with command: ['../../integritee/integritee-node/target/release/integritee-node', '--tmp', '--dev', '-lruntime=info', '-lteerex=debug', '--ws-port', '9944', '--port', '30390', '--rpc-port', '9933', '--ws-external', '--rpc-external']
./integritee-service --clean-reset -P 3000 -p 9944 -r 4490 -w 3001 -h 4546 --ws-external --data-dir /tmp/data-dir2 run --skip-ra --dev --teeracle-interval 10s

I've only changed exchange_rate_oracle.rs to work like a stub instead of reaching to remote service.

From the CI run logs it turns out there is deadlock/rest client issue:

[2023-11-21T08:30:40Z ERROR ita_oracle::oracles::exchange_rate_oracle] Getting exchange rate from https://pro-api.coinmarketcap.com/ failed with Rest client error, trying again in Some(3s).
[2023-11-21T08:30:47Z ERROR itc_rest_client::http_client] SendWithCertificateVerification::execute_send_request received error: IO(Os { code: 11, kind: WouldBlock, message: "Resource deadlock avoided" })
[2023-11-21T08:30:47Z ERROR ita_oracle::oracles::exchange_rate_oracle] Getting exchange rate from https://pro-api.coinmarketcap.com/ failed with Rest client error, trying again in Some(3s).

Maybe it's just a random network issue and it needs retry ? 🤔

@clangenb
Copy link
Contributor

Hmm, I retried it multiple times, and it consistently fails, and I retried master, which works. So it seems to be an issue with this PR, although it is not obvious to me, what problem we experience. I might have time to look at it later this week.

@kziemianek
Copy link
Contributor Author

kziemianek commented Nov 22, 2023

Ok 👍🏼
I managed to get https://pro-api.coinmarketcap.com/ api key and run tests locally again without stub - the test still pass.
I may have different env configuration or the problem is not possible to reproduce on my side.
I failed to run dockerized setup.

@clangenb
Copy link
Contributor

My current guess is that the integration tests simply fail because it is a PR from a fork. Could it be that you don't have set up the API keys in your fork? I am just curious, but I will merge the PR anyhow, after the rest of the CI passes.

@kziemianek
Copy link
Contributor Author

@clangenb I just forked repo - I didn't set my api key in github.

@clangenb
Copy link
Contributor

I see that the teeracle tests has passed now, did you set an api key?

@clangenb clangenb merged commit 60ef825 into integritee-network:master Nov 30, 2023
@clangenb
Copy link
Contributor

Thanks for your contributions! Highly valued!

@kziemianek kziemianek deleted the kz/perform-trusted-operation-return-type branch November 30, 2023 08:02
@kziemianek
Copy link
Contributor Author

kziemianek commented Jul 26, 2024

I see that the teeracle tests has passed now, did you set an api key?

I missed that comment sorry.
No I didn't :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A1-cli Affects cli functionalites B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E5-publicapi PR breaks public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants