Hardcoded PoA admin

[fmorency]

In app.go, the GetPoAAdmin() function is

func GetPoAAdmin() string {
	// used only in e2e testing with interchaintest
	if address := os.Getenv("POA_ADMIN_ADDRESS"); address != "" {
		return address
	}

	// !IMPORTANT: testnet only (reece's addr). Change this to a mainnet address
	return "manifest10r39fueph9fq7a6lgswu4zdsg8t3gxlqdwwncm"
}

We should remove the hardcoded address.

I discovered this while trying to test a local chain using local-ic and the following genesis file

...
                    {
                        "key": "app_state.poa.params.admins",
                        "value": [
                            "manifest1hj5fveer5cjtn4wd6wstzugjfdxzl0xp8ws9ct"
                        ]
                    },
...

I expected manifest1hj5fveer5cjtn4wd6wstzugjfdxzl0xp8ws9ct to be the single PoA Admin without having to set the POA_ADMIN_ADDRESS environment variable. Executing

manifestd --chain-id manifest-2 --keyring-backend os --node tcp://localhost:40531 tx manifest update-params manifest1efd63aw40lxf3n4mhf7dzhjkr453axurm6rp3z:50_000_000,manifest17dh4tstgw4s30ht4lfuc2l6gccqkzttxylw8kq:25_000_000,manifest1gp33pgpx78j7lgjm4422tp80uaxseap6m8fd6s:25_000_000 false 0umfx --from acc0

led to

raw_log: 'failed to execute message; message index: 0: invalid authority; expected
  manifest10r39fueph9fq7a6lgswu4zdsg8t3gxlqdwwncm, got manifest1hj5fveer5cjtn4wd6wstzugjfdxzl0xp8ws9ct'

which was unexpected.

Why can't we initialize the list of PoA Admins from the genesis file and need the POA_ADMIN_ADDRESS environment variable?

The POA_ADMIN_ADDRESS use is also undocumented in #18.

[Reecepbcups]

@fmorency This is specifically for testnet vs mainnet overrides. For mainnet, the main address (multisig) from the team will be used. But if a testnet (like we are doing now is to be used), we want to have an override option for this.

Technically I could get the POA array[0], but this does not seem intuitive vs a clear override needed, which is in the testnet summary here (#15). Since this only affects testnet, and can be removed for mainnet, it does not really make sense to document vs the confusion that would add. So this only affects us not the broader network.

[fmorency]

Thank you for the clarification, @Reecepbcups!

It is fine at this stage, but the hardcoded return should be changed before officially releasing the chain.

Let's keep track of this issue here.

[fmorency]

@Reecepbcups Related to this issue, I am starting a new node with

POA_ADMIN_ADDRESS=manifest1hj5fveer5cjtn4wd6wstzugjfdxzl0xp8ws9ct CHAIN_ID="local-1" HOME_DIR="~/.manifest3" TIMEOUT_COMMIT="2000ms" CLEAN=true sh scripts/test_node.sh

where ~/.manifest3 is an empty folder. Running

➜  manifest-ledger git:(main) ✗ manifestd q poa params

always returns the same PoA Admin no matter what I put in POA_ADMIN_ADDRESS

params:
  admins:
  - manifest10d07y265gmmuvt4z0w9aw880jnsr700jmq3jzm
  allow_validator_self_exit: true

I can reproduce it on a freshly cloned repository. Are you able to reproduce?

Note: The issue doesn't appear to affect functionality, i.e., I am able to execute PoA admin tx with manifest1hj5fveer5cjtn4wd6wstzugjfdxzl0xp8ws9ct

[Reecepbcups]

@fmorency Correct, the POA admins param and POA_ADMIN_ADDRESS override are different things. This is a quirk of the Cosmos-SDK I am working around to fit Manifest use cases. This only affects our testnet and will not be applicable to mainnet (the manifestd q poa params will be the admins used, and multisig will be the POA_ADMIN_ADDRESS override by default).

POA_ADMIN_ADDRESS is the authority for all cosmos sdk modules (the one you set) and one is for controlling the POA module itself. I probably should have named it MASTER_WALLET_OVERRIDE or something for testnet. Just through something together

manifest10d07y265gmmuvt4z0w9aw880jnsr700jmq3jzm is found in the scripts/test_node.sh as a hardcoded mnemonic for either user1 or user2 (pretty sure it is user1).

[fmorency]

I'm writing tests using an x/group Group Policy as the POA_ADMIN_ADDRESS.

Some observations for the sake of posterity

• One can modify the POA module Admins field using a Group Proposal where the proposer is a member of the POA_ADMIN_ADDRESS group. Once executed (by the Group Policy itself), the POA module Admin field will be modified to the new value
• One cannot reset the POA module admin to the original Group Policy using a Group Proposal where the proposer is a member of the POA_ADMIN_ADDRESS group. This is because the POA_ADMIN_ADDRESS Group Policy is no longer the authority of the POA module. However, it is still the authority of the other CosmosSDK modules
• The new POA module admin must be the one to update the POA module parameters to reset the POA module admin to the original Group Policy.

While the POA_ADMIN_ADDRESS is the authority for all SDK modules, it is NOT the authority of the POA module itself (unless specified). The POA module Admin field is the authority of the POA module.

[fmorency]

I'm working on wiring the simulator to the manifest module and found a case related to this issue.

The SDK module authority is hardcoded through the GetPoAAdmin() function during the creation of the manifest-ledger application, and this configuration persists for the application's entire lifecycle. The x/manifest module requires the message authority to be the POA Admin. However, I have no control over the POA Admin from the simulation, as it's hardcoded at the creation of the application.

Thoughts @Reecepbcups?

[fmorency]

As a workaround, I can set POA_ADMIN_ADDRESS in TestFullAppSimulation, e.g.,

r := rand.New(rand.NewSource(config.Seed))
params := simulation.RandomParams(r)
accs := simulationtypes.RandomAccounts(r, params.NumKeys())
poaAdminAddr := accs[0]
err = os.Setenv("POA_ADMIN_ADDRESS", poaAdminAddr.Address.String())

...

// The SDK module address is set in `NewApp`.
bApp := app.NewApp(...)

where accs results in the same accounts as the call to RandomAccounts() in SimulateFromSeed().

I don't like it much but it'll do in the meantime.

[Reecepbcups]

strangelove-ventures/poa#184

[Reecepbcups]

Should update to POA commit d321226154bed00f78e90f3c19fef0539d5e707c for a single authority. Removes need for genesis admin params as well from testnet, e2e, etc

can also remove the entire GetPoAAdmin() function. Env POA_ADMIN_ADDRESS is used on the POA side vs app side. So inherited by default

[fmorency]

Thanks @Reecepbcups. I will make the modifications!