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

feat: Add Oneblock 3 assertions #2109

Merged
merged 23 commits into from
Sep 11, 2023
Merged

feat: Add Oneblock 3 assertions #2109

merged 23 commits into from
Sep 11, 2023

Conversation

outofxxx
Copy link
Contributor

@outofxxx outofxxx commented Sep 7, 2023

fix #2067

This PR implements almost all the functions, but as the final data has not been confirmed yet, there are still some details to be improved. I think it can be reviewed now. The remaining details can be updated by submitting a new PR after these data are confirmed.

The information that needs to be confirmed includes:

  1. Network type of student address
    2. What data content is used to represent the three types of VC in the notion table(confirmed, and update code)

Assertion examples

@outofxxx outofxxx requested a review from a team September 7, 2023 06:34
}
}

fn fetch_data_from_notion(course_type: &OneBlockCourseType) -> Result<OneBlockResponse> {
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered creating notion data provider?


#[derive(Serialize, Deserialize, Debug)]
#[serde(rename_all = "camelCase")]
pub struct OneBlockResponse {
Copy link
Member

Choose a reason for hiding this comment

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

Why we need such wrap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean oneblock folder?

Copy link
Member

Choose a reason for hiding this comment

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

Struct, why don't use serde_json::Value istead, it's flatted anyway.

primitives/core/src/assertion.rs Show resolved Hide resolved
headers.insert("Notion-Version", "2022-06-28");
headers.insert(
AUTHORIZATION.as_str(),
GLOBAL_DATA_PROVIDER_CONFIG.read().unwrap().oneblock_notion_key.clone().as_str(),
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered using try_read ? This should not panic.

"credential_endpoint": "wss://tee-staging.litentry.io"
"credential_endpoint": "wss://tee-staging.litentry.io",
"oneblock_notion_key": "ABCDEFGHIJKLMNOPQRSTUVWXYZ",
"oneblock_notion_url": "https://abc.com"
Copy link
Member

Choose a reason for hiding this comment

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

Can we have real url value here?

Copy link
Collaborator

@Kailai-Wang Kailai-Wang left a comment

Choose a reason for hiding this comment

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

The logic looks ok in general, thanks :)

primitives/core/src/assertion.rs Outdated Show resolved Hide resolved
Comment on lines +183 to +185

// For Oneblock
Oneblock(OneBlockCourseType),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm we have somehow interleaved types:

A13,
A14,
Achainable,
A20,
Oneblock,
...

An argument is tho, to make the codec backwards compatible, so we always add items to the bottom of the enum

Comment on lines +168 to +170
OneBlockCourseType::CourseParticipation => {
let text = self.get_watch_progress(columns);
ONEBLOCK_COURSES_CONTENT[5] == text
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems we are checking if the student watched the last lesson - is it intended? I'm just double checking it because of the name Participation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the Ruleset is if data is "第六课", assertion valid; if data is "第一课"~"第五课", assertion invalid;

Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

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

LGTM.

@BillyWooo BillyWooo merged commit 742495d into dev Sep 11, 2023
28 of 30 checks passed
BillyWooo pushed a commit that referenced this pull request Sep 11, 2023
* one block

* add 3 credential template

* update infos

* update codes

* update oneblock notion data logic

* need to search all linked addresses

* add comments

* add config file

* support all substrate network

* add copyright header

* rename closure

* update handle_stf_call_request

* using new entity name

* anonymize data

* update assertion result

* set notion config

* using all_substrate_web3networks

* make fmt

---------

Co-authored-by: songzhouran <songzhouran@gmail.com>
BillyWooo pushed a commit that referenced this pull request Sep 11, 2023
* one block

* add 3 credential template

* update infos

* update codes

* update oneblock notion data logic

* need to search all linked addresses

* add comments

* add config file

* support all substrate network

* add copyright header

* rename closure

* update handle_stf_call_request

* using new entity name

* anonymize data

* update assertion result

* set notion config

* using all_substrate_web3networks

* make fmt

---------

Co-authored-by: songzhouran <songzhouran@gmail.com>
@outofxxx outofxxx deleted the oneblock branch September 12, 2023 00:18
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.

Create 3 notion assertions for OneBlock+ usecase
5 participants