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

proposed transactions: get_full_accounts needs to notify ChainStore of any proposed transaction chagnes #442

Closed
jcalfee opened this issue Nov 14, 2015 · 11 comments

Comments

@jcalfee
Copy link

jcalfee commented Nov 14, 2015

I can create new proposals, but they do not show up in the ChainStore until I refresh the application. The accounts are subscribed via get_full_accounts.

@theoreticalbts
Copy link
Contributor

Assigning this to @bytemaster who best understands the subscription system

@xeroc
Copy link
Contributor

xeroc commented Jan 20, 2016

This turns out to be an issue of get_full_accounts and not the chainstore ..
Assuming I have this setup:

  • Account fabian-secured requires approval by fabian and secured-by-peermit

get_full_accounts on fabian:

"proposals": [],

get_full_accounts on fabian-secured:

   "proposals": [
                {
                    "required_owner_approvals": [],
                    "required_active_approvals": [
                        "1.2.99212"
                    ],
                    "available_owner_approvals": [],
                    "available_key_approvals": [],
                    "expiration_time": "2016-01-21T09:04:08",
                    "available_active_approvals": [
                        "1.2.99149"
                    ],
                    "id": "1.10.60",
                    "proposed_transaction": {
                        "operations": [
                            [
                                0,
                                {
                                    "fee": {
                                        "amount": 3000000,
                                        "asset_id": "1.3.0"
                                    },
                                    "amount": {
                                        "amount": 10000000,
                                        "asset_id": "1.3.0"
                                    },
                                    "from": "1.2.99212",
                                    "to": "1.2.880",
                                    "extensions": []
                                }
                            ]
                        ],
                        "expiration": "2016-01-21T09:04:08",
                        "ref_block_num": 0,
                        "extensions": [],
                        "ref_block_prefix": 0
                    }
                }
            ],

@svk31
Copy link
Contributor

svk31 commented Jan 20, 2016

I've modified the ChainStore in order to handle incoming proposal objects and add them to the relevant account objects in this commit: cryptonomex/graphene-ui@86cee9e

However, this is not sufficient for proposals that require the approval of multi-sig accounts. The required_approval arrays only contain the parent account, not the actual accounts required.

Say 1.2.1 requires the approval of 1.2.2 and 1.2.3. A proposal for a transfer from 1.2.1 will show up with "required_active_approvals": ["1.2.1"], not ["1.2.2", "1.2.3"], and those two accounts will not have any indication that their approval is required.

Two changes are needed:

  • Include the actual required accounts in the required_*_approvals arrays instead of the parent account
  • Include all relevant proposals in get_full_account (this might get fixed by the above change)

@svk31
Copy link
Contributor

svk31 commented Jan 20, 2016

Setting this to high priority since it's the only thing stopping us from releasing proposed transactions in the GUI, opening up lots of new possibilities with multi-sig etc.

@svk31
Copy link
Contributor

svk31 commented Jan 22, 2016

@theoreticalbts @bytemaster can we get some feedback on this please?

@xeroc
Copy link
Contributor

xeroc commented Jan 25, 2016

Bump

@xeroc xeroc assigned theoreticalbts and unassigned bytemaster Jan 27, 2016
@xeroc
Copy link
Contributor

xeroc commented Jan 27, 2016

I reassigned this bug to @theoreticalbts since I am sure it can be fixed easily by adding those proposals to the get_full_account output require my approval as well as those that concern the account directly.

So it's not so much an issue of the chainstore from what I can tell ..
Let's see if it get's updated automatically once we have the full_account output fixed.
(Assuming it's possible of course)

@theoreticalbts
Copy link
Contributor

This ticket is actually very non-trivial.

The required approvals on the proposal_object can't be changed, they are what they are because that's what's actually used by the blockchain permission system. What's actually needed here is the client needs to know the result of the recursive permission traversal used by the chain.

It is very tempting to simply modify the API call to return an extended object, which has the same fields, but additionally includes the result of the traversal. A similar approach is used in e.g. #253 to add fields indicating the block_id and signing_key to the signed_block.

However there is a critical difference: The signed_block_with_info fields can be derived from the signed_block fields, but an account's authorities (and hence the result of the traversal) are chain state which can change, and there's no good way to push those changes to the client.

I think the correct approach is to implement a new object type which is a (proposal_id, account_id) pair which indicates that account can sign proposal. Then whenever an account authority changes, we re-recurse any proposal.

@xeroc
Copy link
Contributor

xeroc commented Feb 9, 2016

I think the correct approach is to implement a new object type which is a (proposal_id, account_id) pair which indicates that account can sign proposal. Then whenever an account authority changes, we re-recurse any proposal.

Could this be done the other way round, too? So that I can get a list of proposals available for signature/approval by account_id?

@theoreticalbts
Copy link
Contributor

@xeroc : Yep! Querying the relation both ways is definitely useful, so we should add index to query it both ways.

@vikramrajkumar
Copy link
Contributor

This issue was moved to bitshares/bitshares-core#161

pmconrad added a commit to pmconrad/graphene that referenced this issue Dec 22, 2017
Prevent bid_collateral from executing through proposal before hardfork
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants