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: allow attesters to agree on the new targets & support eth #1015

Merged
merged 73 commits into from
Jun 6, 2023

Conversation

MaciejBaj
Copy link
Collaborator

@MaciejBaj MaciejBaj commented May 25, 2023

Summary of changes

@openai: ignore

Acceptance Checklist

What types of changes does your code introduce?
Put an x in the boxes that apply

Dependencies

  • Any dependent changes have been merged and published in downstream modules
  • Any teams which manage the dependencies have been notified of the breaking changes

Bug (non-breaking change which fixes an issue):

  • Have you written tests to protect against future occurrences of the bug?

Feature (non-breaking change which adds functionality)

  • Have you seen it working in a development environment?
  • Have you written tests for happy and unhappy paths?
  • Have you implemented this feature as per the issue acceptance criteria?

Substrate

If this change involves substrate, have you remembered:

  • Storage Migration?
  • RPC methods?
  • Chainspec, both JSON specs & the module?
  • Runtime versioning?
  • Benchmarks?

Breaking change (a feature that would cause existing functionality not to work as expected)

  • Is the breaking change documented?
  • Are your commits fully representative of the change?
  • Has any previous functionality been deprecated and versioned?

CI/CD

  • If introducing new actions, have you looked at the action code for anything spurious?
  • Have you written custom scripts for things that could be actions already on the marketplace?
  • If introducing new actions, have you pegged a static version?

Documentation Update

  • Have you checked other documentation, such as the docs repository?
  • If updating script documentation, have you tested it on both environments?

Further comments

Feel free to explain in further detail your choices if this is a significant change.

Screenshots (Please demonstrate this working in PolkadotJS)

Summary by OpenAI

Release Notes:

  • New Feature: Added support for a new benefit source called SlashTreasury.
  • New Feature: Added a RewardsWriteApi trait with a function to repatriate executor from slash treasury.
  • Refactor: Modified the logic and functionality of various modules related to rewards, attesters, and circuits.
  • Test: Added tests for multiple functions and modules.

"Code changes abound,
Rewards and circuits are found,
Tests ensure sound."

@MaciejBaj MaciejBaj self-assigned this May 25, 2023
@MaciejBaj MaciejBaj requested a review from a team as a code owner May 25, 2023 08:00
@vercel
Copy link

vercel bot commented May 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Jun 6, 2023 11:29am

@github-actions github-actions bot added circuit dependencies 🌳 Pull requests that update a dependency file runtime 🛠️ Associated with runtime rust Pull requests that update Rust code labels May 25, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 25, 2023

✨ Next version based on commits since last release

v1.30.0-rc.0

@github-actions
Copy link
Contributor

github-actions bot commented May 25, 2023

🔎 Subalfred feature checks

✅ pallets/3vm

✅ pallets/account-manager

✅ pallets/attesters

✅ pallets/circuit

✅ pallets/clock

✅ pallets/contracts

✅ pallets/contracts-registry

✅ pallets/evm

✅ pallets/executors

✅ pallets/portal

✅ pallets/rewards

✅ pallets/xdns

✅ runtime/common-pallets

✅ runtime/common-types

✅ runtime/mini-mock

✅ runtime/standalone

✅ runtime/t0rn-parachain

✅ runtime/t3rn-parachain

@github-actions
Copy link
Contributor

github-actions bot commented May 25, 2023

🤖 OpenAI

High-level summary

The changes involve modifications to the logic and functionality of the codebase, including adding new traits, functions, modules, and types, modifying existing types, and introducing new constants. The changes also impact the reward system and introduce a new benefit source. Some changes involve generating private keys for multiple blockchains and adding tests for functions that interact with the Polkadot blockchain API. Therefore, it is crucial to review the changes thoroughly to ensure that they are integrated correctly and do not introduce any bugs or security vulnerabilities.

Table of files and their summaries

File Summary
primitives/src/claimable.rs Adds a new variant to an enum called BenefitSource. The new variant is called SlashTreasury.
primitives/src/rewards.rs Adds a trait RewardsWriteApi with a single function repatriate_executor_from_slash_treasury that takes in an sfx_id and a FullSideEffect object. It also adds a module rewards to the crate.
pallets/circuit/src/square_up/mod.rs Adds support for attesters to agree on new targets and supports eth. It also changes the BenefitSource and CircuitRole values and finalizes XTX for the requester.
runtime/standalone/src/circuit_config.rs Adds a new type Rewards and a constant StartingRepatriationPercentage. It also modifies the ReadSFX and RepatriationPeriod types.
runtime/mini-mock/src/lib.rs Adds a new pallet_circuit module and modifies the pallet_attesters module to support it. It also adds two new constants, StartingRepatriationPercentage and CircuitAccountId, and two new functions, with_polkadot_gateway_record and with_eth_gateway_record, to the ExtBuilder struct.
primitives/src/attesters.rs Introduces a new function ecdsa_pubkey_to_eth_address that converts an ECDSA public key to an Ethereum address. It also modifies the verify_attestation_signature function to check if the attester is attesting to the correct target when the target finality is Ethereum.
pallets/attesters/offchain/generateKeys.js Adds a function to generate random private keys for Substrate, Ethereum, and Bitcoin. The function uses the @polkadot and ethereumjs-util libraries to generate the keys and saves them to a JSON file.
pallets/attesters/offchain/generateKeys.test.js Adds a test for the generate_random_private_keys function, which generates private keys for different blockchains. The test checks that the function returns the expected number of keys and that each key has the required properties.
pallets/attesters/offchain/offchain-attester.test.js Adds tests for four functions that interact with the Polkadot blockchain API. The tests mock the API and check if the expected methods are called.
pallets/circuit/src/square_up/test.rs Changes the source and... (summary incomplete)

Chat with 🤖 OpenAI Bot (@openai)

  • Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
  • Invite the bot into a review comment chain by tagging @openai in a reply.

Code suggestions

  • The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
  • You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Ignoring further reviews

  • Type @openai: ignore anywhere in the PR description to ignore further reviews from the bot.

Files ignored due to filter (10)

Ignored files

  • client/packages/attester/helm/Chart.yaml
  • client/packages/attester/helm/templates/deployment.yaml
  • client/packages/attester/helm/templates/serviceaccount.yaml
  • client/packages/attester/helm/templates/servicemonitor.yaml
  • client/packages/attester/helm/templates/svc.yaml
  • client/packages/attester/helm/values.yaml
  • client/packages/attester/package-lock.json
  • client/packages/attester/package.json
  • client/packages/attester/tsconfig.json
  • client/packages/attester/yarn.lock
Files not summarized due to errors (5)

Failed to summarize

  • client/packages/attester/.eslintrc.js (nothing obtained from openai)
  • client/packages/attester/config/roco.ts (nothing obtained from openai)
  • client/packages/attester/helm/templates/_helpers.tpl (nothing obtained from openai)
  • client/packages/attester/src/connection.ts (nothing obtained from openai)
  • client/packages/attester/src/prometheus.ts (nothing obtained from openai)

In the recent run, only the files that changed from the base of the PR and between c85d33fe0c5c9db449bd91b82b09521ed2111ff8 and 128012736b6252e02a2a204d7600fa314a9cabea commits were reviewed.

Files not reviewed due to errors in the recent run (4)

Failed to review in the last run

  • client/packages/attester/config/local.ts (no response)
  • client/packages/attester/.eslintrc.js (no response)
  • client/packages/attester/helm/templates/_helpers.tpl (no response)
  • client/packages/attester/src/connection.ts (no response)
Files not reviewed due to simple changes (2)

Skipped review in the recent run

  • client/packages/attester/.envrc
  • client/packages/attester/.prettierrc

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

clippy found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

pallets/rewards/src/lib.rs Fixed Show fixed Hide fixed
primitives/src/attesters.rs Fixed Show fixed Hide fixed
primitives/src/attesters.rs Fixed Show fixed Hide fixed
primitives/src/attesters.rs Fixed Show fixed Hide fixed
primitives/src/attesters.rs Fixed Show fixed Hide fixed
primitives/src/attesters.rs Fixed Show fixed Hide fixed
primitives/src/attesters.rs Fixed Show fixed Hide fixed
@MaciejBaj MaciejBaj marked this pull request as ready for review June 1, 2023 15:50
@MaciejBaj MaciejBaj requested a review from 3h4x as a code owner June 1, 2023 15:50
@MaciejBaj MaciejBaj requested a review from petscheit June 1, 2023 15:50

for target in AttestationTargets::<T>::get() {
let mut new_next_batch = BatchMessage::default();
new_next_batch.created = n;

Check warning

Code scanning / clippy

field assignment outside of initializer for an instance created with Default::default()

field assignment outside of initializer for an instance created with Default::default()
created: frame_system::Pallet::<T>::block_number(),
};
let mut new_next_batch = BatchMessage::default();
new_next_batch.created = frame_system::Pallet::<T>::block_number();

Check warning

Code scanning / clippy

field assignment outside of initializer for an instance created with Default::default()

field assignment outside of initializer for an instance created with Default::default()
let slash_treasury_account =
T::TreasuryAccounts::get_treasury_account(TreasuryAccount::Slash);
let slash_treasury_balance = T::Currency::free_balance(&slash_treasury_account);
let repatriation_percentage = Self::repatriation_percentage();

Check warning

Code scanning / clippy

unused variable: `repatriation_percentage`

unused variable: `repatriation_percentage`
@3h4x 3h4x requested a review from palozano June 6, 2023 08:03
Copy link
Contributor

@palozano palozano left a comment

Choose a reason for hiding this comment

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

As @3h4x requested, I reviewed the Rust-related part of the PR. Just a couple of comments.

pallets/attesters/src/lib.rs Show resolved Hide resolved
@3h4x 3h4x enabled auto-merge June 6, 2023 12:39
@MaciejBaj MaciejBaj requested a review from palozano June 6, 2023 14:58
@3h4x 3h4x added this pull request to the merge queue Jun 6, 2023
Merged via the queue into development with commit f4684f0 Jun 6, 2023
@3h4x 3h4x deleted the refactor/attesters-new-targets branch June 6, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
circuit dependencies 🌳 Pull requests that update a dependency file github_actions Pull requests that update GitHub Actions code javascript Pull requests that update Javascript code runtime 🛠️ Associated with runtime rust Pull requests that update Rust code test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants