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(stellar): refactor operators script and add missing commands #540

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ahramy
Copy link
Contributor

@ahramy ahramy commented Feb 9, 2025

AXE-7488

node stellar/operators.js collect_fees CCQSKSL4FSHQT6OF5BS7KHHDUTFSC36BWQH55NFUHWRMMNCRWVRP5GEL --gas-fee-amount 1
node stellar/operators.js refund 0x04593938310ffac16d1d7f11c55cadc1200af00e68ded8b5465558d481685a84-276 CCQSKSL4FSHQT6OF5BS7KHHDUTFSC36BWQH55NFUHWRMMNCRWVRP5GEL```

@ahramy ahramy marked this pull request as ready for review February 9, 2025 21:40
@ahramy ahramy requested a review from a team as a code owner February 9, 2025 21:40
@ahramy ahramy requested review from jcs47 and milapsheth February 9, 2025 21:40
@ahramy ahramy changed the title refactor(stellar): refactor operators script feat(stellar): refactor operators script and add missing commands Feb 9, 2025
stellar/operators.js Outdated Show resolved Hide resolved
stellar/operators.js Outdated Show resolved Hide resolved

if (returnValue.value()) {
printInfo('Return value', returnValue.value());
if (!chain.contracts?.axelar_operators) {
Copy link
Member

Choose a reason for hiding this comment

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

Use validate parameters? Add a soroban- address validation method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added validateParameters and isValidAddress. also created a ticket to refactor other contract addresses: https://axelarnetwork.atlassian.net/browse/AXE-7492

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stellar/operators.js Outdated Show resolved Hide resolved
stellar/operators.js Outdated Show resolved Hide resolved
stellar/operators.js Outdated Show resolved Hide resolved
stellar/operators.js Outdated Show resolved Hide resolved
stellar/utils.js Outdated Show resolved Hide resolved
Comment on lines +123 to 125
if (!isValidAddress(contractAddress)) {
throw new Error('Invalid operators contract');
}
Copy link
Member

Choose a reason for hiding this comment

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

This defeats the point of validateParameters. Take a look at it's implementation. We want to add support for stellar address validation to it

validateParameters({
    isValidStellarAddress: { contractAddress },
});

Copy link
Member

@milapsheth milapsheth Feb 10, 2025

Choose a reason for hiding this comment

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

Could specialize into two functions as well
isStellarContract and isStellarAccount, since these addresses start with C and G respectively for stricter validation

Copy link
Contributor Author

@ahramy ahramy Feb 10, 2025

Choose a reason for hiding this comment

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

I think we can work on this on a separate PR since this task may be out of scope and could impact other scripts. I've created a ticket to track it: https://axelarnetwork.atlassian.net/browse/AXE-7492

stellar/operators.js Outdated Show resolved Hide resolved
stellar/operators.js Show resolved Hide resolved
stellar/operators.js Outdated Show resolved Hide resolved
ahramy and others added 2 commits February 9, 2025 23:34
@@ -340,6 +340,16 @@ function stellarAddressToBytes(address) {
return hexlify(Buffer.from(address, 'ascii'));
}

function isValidAddress(address) {
try {
// try conversion
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: implied with try block

program.action((options) => {
const config = loadConfig(options.env);
processCommand(options, config, getChainConfig(config, options.chainName));
program.command('remove-operator <addrsess>').action((address, options) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
program.command('remove-operator <addrsess>').action((address, options) => {
program.command('remove-operator <address>').action((address, options) => {

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