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

Update for Paseo support #187

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Update for Paseo support #187

merged 2 commits into from
Mar 27, 2024

Conversation

wilwade
Copy link
Contributor

@wilwade wilwade commented Mar 26, 2024

Goal

The goal of this PR is to add the Frequency Paseo Testnet support to the Graph SDK

Copy link
Collaborator

@aramikm aramikm 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, some nits and a comment to fix the ci issue

bridge/common/protos/output.proto Outdated Show resolved Hide resolved
"graphPublicKeySchemaId": 7,
"schemaMap": [
[
8,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wanted to make sure that these schema id's are the same as what we registered on paseo for each

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are. Here's the output of the deploy script:

Generated schema mapping:
 {
  "0x203c6838fc78ea3660a2f298a58d859519c72a5efdc0f194abd6f0d5ce1838e0": {
    "tombstone": {
      "1.2": 1
    },
    "broadcast": {
      "1.2": 2
    },
    "reply": {
      "1.2": 3
    },
    "reaction": {
      "1.1": 4
    },
    "profile": {
      "1.2": 5
    },
    "update": {
      "1.2": 6
    },
    "public-key-key-agreement": {
      "1.2": 7
    },
    "public-follows": {
      "1.2": 8
    },
    "private-follows": {
      "1.2": 9
    },
    "private-connections": {
      "1.2": 10
    },
    "public-key-assertion-method": {
      "1.3": 11
    }
  }
}

}
]
]
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the paseo schema 10 does not have any settings , expecting signature required setting on it ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with schema 9 it has setting set to 0 , I think private schemas require signatures enforcement

Copy link
Collaborator

Choose a reason for hiding this comment

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

schema 8 is good it's Avro Paginated and setting=0

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think schema 7 has setting = 3 not sure if it corresponds to AppendOnly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the same settings as appear on mainnet. So if they are wrong, so is mainnet 😟

Copy link
Collaborator

@saraswatpuneet saraswatpuneet Mar 27, 2024

Choose a reason for hiding this comment

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

Yeah I think they are wrong "in spirit" meaning most stateful operations everywhere is only calling "upsert_page" extrinsic and it does not check for schema permission but extrinsics with "...._with_signature" might be misleading

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its, fine but any application that enforces signature permission on schemas should know, not to use the ones in this config,

https://github.com/LibertyDSNP/frequency/blob/9b948d72751839e516791a4cb3ee216985554963/pallets/stateful-storage/src/lib.rs#L770C47-L770C64

@aramikm
Copy link
Collaborator

aramikm commented Mar 26, 2024

Might want to add a test like getGraphConfig with Rococo environment should return the graph config for paseo in node bridge

Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

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

lgtm! schema related questions does not affect this PR

@wilwade wilwade merged commit 91423f6 into main Mar 27, 2024
11 checks passed
@wilwade wilwade deleted the paseo-update branch March 27, 2024 16:58
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.

3 participants