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

Governance Service to Audit #3

Merged
merged 118 commits into from
Feb 9, 2024
Merged

Conversation

ryan-suematsu-anchain
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@franklywatson franklywatson left a comment

Choose a reason for hiding this comment

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

Some minor comments but seems ok so far

cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved
cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

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

Left a number of suggestions. Looks like AxelarGovernanceService has some todos, though ofc not opposed to approving before those are complete. Most comments are syntax suggestions, though there are a couple of questions that arose around unstated assumptions I think would be helpful to clear up.

  • What scale do we expect to see in the contract mapping fields? In the case of this PR, AxelarGovernanceService.proposals, though I did see a couple in the gateway contract.
  • How critical are the senderAddress method parameters throughout the contracts and are there any places where DDOS and by extension impersonation (perhaps two separate concerns) pose critical vulnerabilities to the messaging protocol
  • How are payments to the gas service accounted for? I ask because I don't see state tracking onchain, so I'm assuming this is a relevant detail for the Axelar network?

cadence/transactions/service-contracts/gas-service-add.cdc Outdated Show resolved Hide resolved
cadence/transactions/service-contracts/gas-service-add.cdc Outdated Show resolved Hide resolved
cadence/transactions/service-contracts/gas-service-add.cdc Outdated Show resolved Hide resolved
cadence/transactions/service-contracts/gas-service-add.cdc Outdated Show resolved Hide resolved
cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved
cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved
cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved
cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved
@sisyphusSmiling
Copy link
Contributor

Quick follow up, I'm wondering about plans for building out a unit testing suite. Np if we just want to align on the initial design and implementation, but it'd be helpful for reviews since I lack the full context around business logic.

@sisyphusSmiling sisyphusSmiling mentioned this pull request Oct 30, 2023
cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved
cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved
cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved
// TODO: Replace update__experimental with tryUpdate() once it's available
// let deploymentResult = account.contracts.tryUpdate(name: name, code: code)
// return deploymentResult.success
account.contracts.update__experimental(name: name, code: code)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just flagging that this should be updated once tryUpdate is available. The commented code should cover the use case based on the planned interface.

Relevant issue: onflow/cadence#2963

cadence/contracts/services/AxelarGovernanceService.cdc Outdated Show resolved Hide resolved


//Get estimated execution time for proposal
access(all) fun getProposalEta(proposedCode: String, target: Address): UInt64{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also think we'd want to add a getter for Proposals so they can be reivewed. Also, will the hashed proposal values be stored somewhere offchain e.g. on Axelar somewhere. Asking bc I think indexing on the proposed code + target address could obscure the values unless the hashed values are known and meaning assigned elsewhere.

tests/transactions/publish-auth-capability.ts Outdated Show resolved Hide resolved
tests/transactions/setup-governance-updater.ts Outdated Show resolved Hide resolved
ryan-suematsu-anchain and others added 27 commits January 10, 2024 22:23
setup governance updter file'
@ryan-suematsu-anchain ryan-suematsu-anchain merged commit b588109 into main Feb 9, 2024
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.

4 participants