Skip to content

feat: set impossible lock_transfer_block_id #2316

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 11, 2021
Merged

feat: set impossible lock_transfer_block_id #2316

merged 5 commits into from
Jan 11, 2021

Conversation

@zone117x
Copy link
Contributor

@zone117x zone117x commented Jan 11, 2021

@yknl

For the wallet issue that I mentioned during stand up, one way we can disable outgoing transactions from the wallet when we enter the freeze is to return a lock_transfer_block_id greater than the current block from the account status API endpoint on core:
https://core.blockstack.org/v1/accounts/1JSwmo6g8zgpKPMVRcrJBeH2oqJE8N9rax/STACKS/status

example return with lock_transfer_block_id set to 9999999:

address: "1JSwmo6g8zgpKPMVRcrJBeH2oqJE8N9rax"
block_id: 663762
credit_value: "1591022"
debit_value: "1270011"
lock_transfer_block_id: 9999999
txid: "e1d2702b865e180133690dd3dc022085168bf680f943853c53968572ed0a8d6c"
type: "STACKS"
vtxindex: 2263

@zone117x zone117x requested review from jcnelson, kantai and yknl January 11, 2021 18:23
@zone117x zone117x self-assigned this Jan 11, 2021
yknl
yknl approved these changes Jan 11, 2021
View reviewed changes
Copy link
Contributor

@yknl yknl left a comment

Choose a reason for hiding this comment

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

Thanks @zone117x Looks good to me!

zone117x
zone117x commented Jan 11, 2021
View reviewed changes
blockstack/blockstackd.py
def export_account_state(self, account_state):
"""
Make an account state presentable to external consumers
"""
Copy link
Contributor Author

@zone117x zone117x Jan 11, 2021

Choose a reason for hiding this comment

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

@jcnelson @kantai this location for a patch was not intuitive for me. There was another area that made more sense, but found this after some local testing. Please confirm

Copy link
Contributor

@kantai kantai Jan 11, 2021

Choose a reason for hiding this comment

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

I think @jcnelson has the most context on the appropriate place to apply this. Alternatively, you can catch the forwarding that happens in api/server.py and rewrite the lock time there.

Copy link
Member

@jcnelson jcnelson Jan 11, 2021

Choose a reason for hiding this comment

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

All this change will do is alter what is (a) returned by the RESTful API and (b) what is returned by blockstack/lib/client.py. If that's what you want, then this is fine. I think it's more appropriate to do it here than in api/server.py.

diwakergupta
diwakergupta reviewed Jan 11, 2021
View reviewed changes
blockstack/blockstackd.py Outdated
'txid': account_state['txid'],
}
# if block height is after the migration export threshold, return a lock height that will force the wallet to error when sending a tx
if result['block_id'] >= 665750:
Copy link
Contributor

@diwakergupta diwakergupta Jan 11, 2021

Choose a reason for hiding this comment

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

Can you use the constant IMPORT_BLOCK defined in Aaron's PR?

Copy link
Contributor Author

@zone117x zone117x Jan 11, 2021

Choose a reason for hiding this comment

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

idk how to import that from the other file. @kantai

Copy link
Contributor

@diwakergupta diwakergupta Jan 11, 2021

Choose a reason for hiding this comment

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

Not critical if that's not straight forward.

Copy link
Contributor Author

@zone117x zone117x Jan 11, 2021

Choose a reason for hiding this comment

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

Probably is but python module import is cancer

@kantai kantai changed the base branch from v1-migration to feat/hardcode-export-ht January 11, 2021 18:42
@kantai kantai changed the base branch from feat/hardcode-export-ht to v1-migration January 11, 2021 18:46
@jcnelson
Copy link
Member

jcnelson commented Jan 11, 2021 •
edited
Loading

I'm a little confused as to what this is supposed to do. Is it just supposed to trick the 1.0 wallet into thinking it can't spend STX because the tokens are all "locked"? If so, then the changes proposed here should be fine, but you'll want to of course test them with the 1.0 wallet.

@kantai
Copy link
Contributor

kantai commented Jan 11, 2021

I'm a little confused as to what this is supposed to do. Is it just supposed to trick the 1.0 wallet into thinking it can't spend STX because the tokens are all "locked"? If so, then the changes proposed here should be fine, but you'll want to of course test them with the 1.0 wallet.

Yep -- that's the idea.

I think this PR needs a slight tweak -- the block_id checked here isn't the current block height, but rather the block_id that the account was last modified at, correct?

@kantai
Copy link
Contributor

kantai commented Jan 11, 2021

Tested locally with an already passed block height, and this works correctly.

@kantai kantai merged commit a656c14 into v1-migration Jan 11, 2021
@blockstack-devops
Copy link
Contributor

blockstack-devops commented Nov 26, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jcnelson jcnelson jcnelson approved these changes

+3 more reviewers

@diwakergupta diwakergupta diwakergupta left review comments

@kantai kantai kantai approved these changes

@yknl yknl yknl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@zone117x zone117x

Labels

locked

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@zone117x @jcnelson @kantai @blockstack-devops @diwakergupta @yknl