Skip to content

feat(satp-hermes): improve satp gateway configuration object #4079

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(satp-hermes): improve satp gateway configuration object #4079

wants to merge 1 commit into from

Conversation

@Tomas-Silva2187
Copy link
Contributor

@Tomas-Silva2187 Tomas-Silva2187 commented Nov 18, 2025 •
edited
Loading

feat(satp-hermes): improve satp gateway configuration object

Implements the required improvements to the satp gateway object.

Added support for gateways, in their own credentials for identification, and along with their public key, to share the cryptographic algorithm they used to generate their own key pair.

CLOSES #4013

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

@Tomas-Silva2187 Tomas-Silva2187 changed the title First version of changes feat(satp-hermes): improve satp gateway configuration object Nov 18, 2025
@Tomas-Silva2187 Tomas-Silva2187 marked this pull request as ready for review November 19, 2025 12:03
@Tomas-Silva2187 Tomas-Silva2187 force-pushed the bridgeConfig11-11 branch 9 times, most recently from 7756dd9 to 3a05cb9 Compare November 21, 2025 12:40
RafaelAPB
RafaelAPB requested changes Nov 21, 2025
View reviewed changes
Copy link
Contributor

@RafaelAPB RafaelAPB left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, left comments directed at minor issues to be addressed

@Tomas-Silva2187 Tomas-Silva2187 force-pushed the bridgeConfig11-11 branch 3 times, most recently from 5060b5c to 553bf7c Compare November 22, 2025 12:51
LordKubaya
LordKubaya requested changes Nov 24, 2025
View reviewed changes
Copy link
Contributor

@LordKubaya LordKubaya left a comment

Choose a reason for hiding this comment

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

Please add to the description that you added support for different type of keys.

packages/cactus-plugin-satp-hermes/README.md Outdated
"connectorUrl": "https://besu-connector.example.com:4000",
"web3SigningCredential": {
"ethAccount": "${ETH_ACCOUNT}",
"bridgeOwnerAddress": "${ETH_ACCOUNT}",
Copy link
Contributor

@LordKubaya LordKubaya Nov 24, 2025

Choose a reason for hiding this comment

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

Is this right? bridgeOwnerAddress in the type web3SigningCredential?
What is the reason for changing this?

Copy link
Contributor Author

@Tomas-Silva2187 Tomas-Silva2187 Nov 24, 2025

Choose a reason for hiding this comment

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

The issue relative to this PR requests the ethAccount parameter to have a clearer name. I've changed it now to "bridgeOwnerAccount", which may be clearer, and more technically correct.

Copy link
Contributor

@LordKubaya LordKubaya Nov 25, 2025

Choose a reason for hiding this comment

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

But the web3SigningCredential is about the EVM-based systems. This can be used in other systems that are not bridges.

packages/cactus-plugin-satp-hermes/README.md Outdated
},
signingCredential: {
ethAccount: "0x736dC9B8258Ec5ab2419DDdffA9e1fa5C201D0b4", // Ethereum account address
bridgeOwnerAddress: "0x736dC9B8258Ec5ab2419DDdffA9e1fa5C201D0b4", // Ethereum account address of the owner of the bridge
Copy link
Contributor

@LordKubaya LordKubaya Nov 24, 2025

Choose a reason for hiding this comment

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

This is a key to sign things that are not related to the ETH

Copy link
Contributor Author

@Tomas-Silva2187 Tomas-Silva2187 Nov 24, 2025

Choose a reason for hiding this comment

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

I've addressed it, and given a name that is more technically correct, as I commented above.

packages/cactus-plugin-satp-hermes/README.md Outdated
},
signingCredential: {
ethAccount: "0x09D16c22216BC873e53c8D93A38420f48A81dF1B", // Ethereum account address
bridgeOwnerAddress: "0x09D16c22216BC873e53c8D93A38420f48A81dF1B", // Ethereum account address
Copy link
Contributor

@LordKubaya LordKubaya Nov 24, 2025

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

@Tomas-Silva2187 Tomas-Silva2187 Nov 24, 2025

Choose a reason for hiding this comment

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

Addressed. Explanation above.

...-satp-hermes/src/test/typescript/unit/config-validating-functions/validate-cc-config.test.ts Outdated
},
signingCredential: {
ethAccount: "0x8230f81920ed354445d201222470ad6f92459D3f",
bridgeOwnerAddress: "0x8230f81920ed354445d201222470ad6f92459D3f",
Copy link
Contributor

@LordKubaya LordKubaya Nov 24, 2025

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

@Tomas-Silva2187 Tomas-Silva2187 Nov 24, 2025

Choose a reason for hiding this comment

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

Addressed. Explanation above.

@Tomas-Silva2187 Tomas-Silva2187 force-pushed the bridgeConfig11-11 branch 3 times, most recently from 4afe45b to 7065667 Compare November 24, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LordKubaya LordKubaya LordKubaya requested changes

@RafaelAPB RafaelAPB RafaelAPB approved these changes

@VRamakrishna VRamakrishna Awaiting requested review from VRamakrishna VRamakrishna is a code owner

@sandeepnRES sandeepnRES Awaiting requested review from sandeepnRES sandeepnRES is a code owner

@outSH outSH Awaiting requested review from outSH outSH is a code owner

@AndreAugusto11 AndreAugusto11 Awaiting requested review from AndreAugusto11 AndreAugusto11 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat(satp-hermes): improve satp gateway configuration object

3 participants

@Tomas-Silva2187 @RafaelAPB @LordKubaya