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: validate horizon staking authorization during agent bootup #95

Merged
merged 20 commits into from
Nov 25, 2024

Conversation

jahabeebs
Copy link
Collaborator

🤖 Linear

Closes GRT-231

Description

  • Validates Horizon Staking authorization during agent bootup

@jahabeebs jahabeebs self-assigned this Nov 22, 2024
Copy link

linear bot commented Nov 22, 2024

@jahabeebs jahabeebs changed the base branch from dev to feat/grt-230-update-rpc-abis-and-behavior November 22, 2024 02:56
@jahabeebs jahabeebs marked this pull request as ready for review November 22, 2024 03:21
0xyaco
0xyaco previously approved these changes Nov 22, 2024
Copy link
Collaborator

@0xyaco 0xyaco left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -16,6 +16,7 @@ protocolProvider:
eboRequestCreator: "0xa13318684281a820304C164427396385C306d870"
bondEscalationModule: "0x52d7728fE87826FfF51b21b303e2FF7cB04F6Aec"
horizonAccountingExtension: "0xbDAB27D1903da4e18B0D1BE873E18924514E52eC"
horizonStaking: "0x1234567890123456789012345678901234567890" # TODO: Update address
Copy link
Collaborator

Choose a reason for hiding this comment

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

This address should be fine for the moment (Sepolia's Horizon Staking contract address):

0x3F53F9f9a5d7F36dCC869f8D2F227499c411c0cf

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

replaced 👍🏻

Copy link
Collaborator

@0xnigir1 0xnigir1 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, just a few comments to double check some things 🫡

@@ -42,6 +42,7 @@ const contractsAddressesSchema = z.object({
horizonAccountingExtension: hexSchema
.optional()
.default("0x0000000000000000000000000000000000000000"),
horizonStaking: hexSchema,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we use hexSchema here but on config we use addressSchema?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you're right, I ended up changing hexSchema to addressSchema. However I don't want to abstract out the zod types to the shared package or create a circular dependency so I have a duplicate addressSchema for these two packages for now

Comment on lines 1250 to 1255
const authorized = await this.horizonStakingContract.read.isAuthorized([
serviceProvider,
verifier,
operator,
]);
return authorized;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const authorized = await this.horizonStakingContract.read.isAuthorized([
serviceProvider,
verifier,
operator,
]);
return authorized;
return this.horizonStakingContract.read.isAuthorized([
serviceProvider,
verifier,
operator,
]);

picky here 😆 but given that you only return the result without logging or try/catch block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored 👍🏻

if (!isAuth) {
const errorMessage = `Authorization required: Operator ${operator} is not authorized for service provider ${serviceProvider}.`;
this.logger.error(errorMessage);
process.exit(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

just double checking that we want to exit the process right away

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yepp

Base automatically changed from feat/grt-230-update-rpc-abis-and-behavior to dev November 22, 2024 17:58
@jahabeebs jahabeebs dismissed 0xyaco’s stale review November 22, 2024 17:58

The base branch was changed.

…zon-auth-during-bootup

# Conflicts:
#	apps/agent/config.example.yml
#	packages/automated-dispute/src/providers/protocolProvider.ts
#	packages/automated-dispute/tests/services/protocolProvider.spec.ts
@jahabeebs jahabeebs requested a review from 0xnigir1 November 22, 2024 22:24
@jahabeebs jahabeebs requested a review from 0xyaco November 22, 2024 22:24

describe("start", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this describe deleted? 🤔

0xnigir1
0xnigir1 previously approved these changes Nov 25, 2024
@jahabeebs jahabeebs requested a review from 0xyaco November 25, 2024 14:42
@jahabeebs jahabeebs merged commit 6575d77 into dev Nov 25, 2024
5 checks passed
@jahabeebs jahabeebs deleted the feat/231-validate-horizon-auth-during-bootup branch November 25, 2024 14:57
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