Executor Contracts by etzellux · Pull Request #3 · tinymanorg/tinyman-governance

etzellux · 2023-11-16T08:51:39Z

https://github.com/Hipo/private-tinyman-py-sdk/tree/executors

gokselcoban

I checked the contract part for now. It looks good overall, thank you!

contracts/arbitrary_executor/arbitrary_executor_approval.tl

gokselcoban · 2023-11-23T08:47:06Z

contracts/arbitrary_executor_logic_signature/arbitrary_executor_logic_signature.tl

+assert(Txn.GroupIndex == 1)
+
+# Assert that there is no rekeying
+assert(Gtxn[1].RekeyTo == Global.ZeroAddress)


Using the Txn instead of Gtxn[1] will improve the readability.

Suggested change

assert(Gtxn[1].RekeyTo == Global.ZeroAddress)

assert(Txn.RekeyTo == Global.ZeroAddress)

contracts/arbitrary_executor_logic_signature/arbitrary_executor_logic_signature.tl

gokselcoban · 2023-11-23T08:57:11Z

contracts/arbitrary_executor_logic_signature/arbitrary_executor_logic_signature.tl

+#tealish version git+https://github.com/Hipo/tealish.git@e8d1b27620220bc4e520d7d3b6d62523e13a7723
+
+# Assert that Gtxn[0] is an appcall to execution validator
+assert(Gtxn[0].ApplicationID == 10000)


Maybe the app id should be a parameter/variable. I couldn't be sure.

gokselcoban · 2023-11-23T09:01:39Z

contracts/proposal_voting/proposal_voting_approval.tl

-        log(Concat(method("proposal(byte[59],uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,bool,bool,bool,bool,address)"), proposal_id, proposal_data))
+        log(Concat(method("proposal(byte[59],uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,bool,bool,bool,bool,address, byte[128])"), proposal_id, proposal_data))
        log(Concat(method("create_proposal(address,byte[59])"), user_address, proposal_id))
+        log(proposal.execution_hash)


I guess this is added for testing purposes.

gokselcoban · 2023-11-23T09:08:25Z

contracts/proposal_voting/proposal_voting_approval.tl

        bytes proposal_data
        _, proposal_data = box_get(proposal_box_name)
-        log(Concat(method("proposal(byte[59],uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,bool,bool,bool,bool,address)"), proposal_id, proposal_data))
+        log(Concat(method("proposal(byte[59],uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,bool,bool,bool,bool,address, byte[128])"), proposal_id, proposal_data))


FYI. There are other logs that should be updated, too.

Maybe we can handle it together with state change.
#1 (comment)

contracts/arbitrary_executor/arbitrary_executor_approval.tl

gokselcoban

Great work, it looks clear!

gokselcoban · 2023-11-28T15:07:53Z

contracts/fee_management_executor/fee_management_executor_approval.tl

+        # Assert proposal.execution_hash
+        # hash(method_name, pool_address: address, total_fee_share, protocol_fee_ratio)
+
+        bytes execution_hash = Lpad(sha256(Concat(Concat(Concat("set_fee_for_pool", Txn.Accounts[1]), total_fee_share), protocol_fee_ratio)), 128)


FYI: Concat supports multiple parameters, thanks to Tealish.

Suggested change

bytes execution_hash = Lpad(sha256(Concat(Concat(Concat("set_fee_for_pool", Txn.Accounts[1]), total_fee_share), protocol_fee_ratio)), 128)

bytes execution_hash = Lpad(sha256(Concat("set_fee_for_pool", Txn.Accounts[1], total_fee_share, protocol_fee_ratio)), 128)

gokselcoban · 2023-11-28T15:11:21Z

contracts/treasury_management_executor/treasury_management_executor_approval.tl

+        # Assert proposal.execution_hash
+        # hash(method_name, address, amount)
+
+        bytes execution_hash = Lpad(sha256(Concat(Concat("send_algo", Txn.Accounts[0]), amount_as_bytes), 128)


FYI: Maybe we can convert amount (amount_as_bytes) to int first and then convert it to byte to make sure it is 8 bytes.

gokselcoban · 2023-11-28T15:15:39Z

contracts/treasury_management_executor/treasury_management_executor_approval.tl

+        exit(1)
+    end
+
+    block send_asa:


FYI: Maybe we can determine the transaction type (Pay or Axfer) according to asset_id. Two method is not required in this case.

gokselcoban · 2023-11-28T15:19:34Z

contracts/treasury_management_executor/treasury_management_executor_approval.tl

+
+        inner_txn:
+            TypeEnum: Axfer
+            Sender: Global.CurrentApplicationAddress


To make it clear.

We should set the fee collector address to the app account.

If we want to continue using the same executor app, if we change the fee collector, we can rekey the fee collector to the app account. In this case, we should send the transaction on behalf of the fee collector. The Sender parameter should be added to the hash.

I aggree, the fee collector and treasury management app is conflicting in this manner.

gokselcoban

This looks good overall!

gokselcoban · 2023-12-08T13:34:20Z

contracts/arbitrary_executor/arbitrary_executor_approval.tl

+        # proposal checks
+        assert(proposal_state == PROPOSAL_STATE_SUCCEEDED)
+
+        # Assert Gtxn[1]'s transaction_id.


Suggested change

# Assert Gtxn[1]'s transaction_id.

gokselcoban · 2023-12-08T13:45:35Z

contracts/arbitrary_executor/arbitrary_executor_logic_signature.tl

+# Assert that there is no rekeying
+int start = 0
+int end = Global.GroupSize
+for i in start:end:
+    assert(Gtxn[i].RekeyTo == Global.ZeroAddress)
+end


Ideally, Txn.RekeyTo == Global.ZeroAddress) should be enough.

Governors should know the TXN fields before approving it.

gokselcoban · 2023-12-08T14:08:34Z

contracts/treasury_management_executor/treasury_management_executor_approval.tl

+        else:
+            exit(0)
+        end


FYI: There is no signed integer in the AVM. You can safely update elif as else.

gokselcoban · 2023-12-08T14:09:04Z

tests/common.py

+def lpad(string: bytes, n: int) -> bytes:
+    assert(n > 0)
+
+    return b"\x00" * (128 - len(string)) + string


Suggested change

return b"\x00" * (128 - len(string)) + string

return b"\x00" * (n - len(string)) + string

etzellux · 2024-02-07T09:29:45Z

Continueing on #7

etzellux added 7 commits November 15, 2023 17:51

init arbitrary executor

7f10e0b

fix cast error in arbitrary executor

fe7afb7

add arbitrary_executor_clear_state

ab56782

add initial test

64f15a1

add test_execute_proposal

dee010a

update test_execute_proposal

092ca1b

add executor logic signature

2007c4f

gokselcoban reviewed Nov 23, 2023

View reviewed changes

etzellux added 4 commits November 23, 2023 12:26

add fee_management_executor contracts

f6515fb

add fee_management_executor tests

6e20eed

update test_set_fee_for_pool_execution

6201f0d

add treasury_management_executor contracts

fe72cd9

gokselcoban reviewed Nov 28, 2023

View reviewed changes

etzellux added 8 commits November 29, 2023 17:47

add TreasuryManagementExecutorTestCase

e25bcdd

update test and executors

1bb0053

remove sha256 from arbitrary methods

dd5fb4c

update fee_management_executor and tests

5362922

update treasury_management_executor and tests

e002106

update arbitrary_executor_logic_signature

260330c

add set_proposal_as_executed to executors

099172a

update tests with sdk functions

9058eee

gokselcoban reviewed Dec 8, 2023

View reviewed changes

minor updates on contracts and tests

4217e5d

etzellux mentioned this pull request Feb 6, 2024

Update Executor Contracts #7

Open

etzellux marked this pull request as draft February 6, 2024 12:23

etzellux closed this Feb 7, 2024

	assert(Gtxn[1].RekeyTo == Global.ZeroAddress)
	assert(Txn.RekeyTo == Global.ZeroAddress)

	bytes execution_hash = Lpad(sha256(Concat(Concat(Concat("set_fee_for_pool", Txn.Accounts[1]), total_fee_share), protocol_fee_ratio)), 128)
	bytes execution_hash = Lpad(sha256(Concat("set_fee_for_pool", Txn.Accounts[1], total_fee_share, protocol_fee_ratio)), 128)

	return b"\x00" * (128 - len(string)) + string
	return b"\x00" * (n - len(string)) + string

Conversation

etzellux commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gokselcoban left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gokselcoban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etzellux Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gokselcoban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etzellux commented Feb 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

etzellux commented Nov 16, 2023 •

edited

Loading

etzellux Nov 29, 2023 •

edited

Loading