Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

Problem (Fix #1515): unbond tx don't subtract fee from bonded #1516

Merged
merged 1 commit into from
May 1, 2020

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Apr 30, 2020

Solution:

  • Fix the bug and add unit test

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #1516 into master will increase coverage by 0.02%.
The diff coverage is 92.10%.

@@            Coverage Diff             @@
##           master    #1516      +/-   ##
==========================================
+ Coverage   65.75%   65.77%   +0.02%     
==========================================
  Files         161      161              
  Lines       21789    21808      +19     
==========================================
+ Hits        14328    14345      +17     
- Misses       7461     7463       +2     
Impacted Files Coverage Δ
...work/src/network_ops/default_network_ops_client.rs 78.03% <66.66%> (-0.22%) ⬇️
chain-abci/src/app/mod.rs 83.09% <100.00%> (+0.07%) ⬆️
chain-abci/src/app/staking_event.rs 99.37% <100.00%> (+<0.01%) ⬆️
chain-abci/src/staking/mod.rs 99.10% <100.00%> (+0.01%) ⬆️
chain-abci/src/staking/tx.rs 91.96% <100.00%> (+0.14%) ⬆️
chain-abci/src/storage/mod.rs 84.69% <100.00%> (+0.08%) ⬆️
chain-abci/tests/abci_app.rs 94.87% <100.00%> (+0.02%) ⬆️

leejw51
leejw51 approved these changes Apr 30, 2020
Copy link
Collaborator

@leejw51crypto leejw51crypto left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request May 1, 2020
1511: Problem (Fix #1313): light client doesn't verify the fetched staking state r=tomtau a=yihuang

Solution:
- Add staking_root in sync state
- Remove the "account" path in favor of the new "staking" path
- Client staking state command add wallet name parameter,
  and verify staking state and proof against the trusted staking_root in sync state
- Need to sync wallet before fetching the new staking state now. (you can still get any staking state without verification by request abci_query API directly)

1516: Problem (Fix #1515): unbond tx don't subtract fee from bonded r=tomtau a=yihuang

Solution:
- Fix the bug and add unit test


Co-authored-by: yihuang <huang@crypto.com>
@bors
Copy link
Contributor

bors bot commented May 1, 2020

Build failed (retrying...):

@tomtau
Copy link
Contributor

tomtau commented May 1, 2020

bors retry

@bors
Copy link
Contributor

bors bot commented May 1, 2020

Already running a review

@tomtau
Copy link
Contributor

tomtau commented May 1, 2020

bors r+

bors bot added a commit that referenced this pull request May 1, 2020
1516: Problem (Fix #1515): unbond tx don't subtract fee from bonded r=tomtau a=yihuang

Solution:
- Fix the bug and add unit test


Co-authored-by: yihuang <huang@crypto.com>
@bors
Copy link
Contributor

bors bot commented May 1, 2020

Build failed:

@tomtau tomtau self-requested a review May 1, 2020 03:09
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

needs extra sync / wait in multi-node join test?

Traceback (most recent call last):
138	  File "./multinode/join_test.py", line 89, in <module>
139	    txid = rpc.staking.unbond(addr, int(state['bonded']), enckey=enckey, name='target')
140	  File "/drone/src/integration-tests/bot/chainrpc.py", line 208, in unbond
141	    return self.call('staking_unbondStake', [name, enckey or get_enckey()], fix_address(address), str(amount))
142	  File "/drone/src/integration-tests/bot/chainrpc.py", line 68, in call
143	    rsp = request(self.client_rpc_url, method, *args, **kwargs)
144	  File "/drone/src/drone/venv/lib/python3.8/site-packages/jsonrpcclient/__init__.py", line 8, in request
145	    return HTTPClient(endpoint).request(*args, **kwargs)
146	  File "/drone/src/drone/venv/lib/python3.8/site-packages/apply_defaults/decorators.py", line 13, in wrapper
147	    return function(self, *args, **kwargs)
148	  File "/drone/src/drone/venv/lib/python3.8/site-packages/jsonrpcclient/client.py", line 229, in request
149	    return self.send(
150	  File "/drone/src/drone/venv/lib/python3.8/site-packages/apply_defaults/decorators.py", line 13, in wrapper
151	    return function(self, *args, **kwargs)
152	  File "/drone/src/drone/venv/lib/python3.8/site-packages/jsonrpcclient/client.py", line 177, in send
153	    raise ReceivedErrorResponseError(response.data)
154	jsonrpcclient.exceptions.ReceivedErrorResponseError: Invalid input: Staking account does not have enough coins to unbond (synchronizing your wallet may help)

@tomtau
Copy link
Contributor

tomtau commented May 1, 2020

or needs to call

 rpc.staking.unbond(addr, int(state['bonded'] - fee), enckey=enckey, name='target')

instead of

 rpc.staking.unbond(addr, int(state['bonded']), enckey=enckey, name='target')

?

(or something like state['bonded']-min_required+1 ?)

@yihuang
Copy link
Collaborator Author

yihuang commented May 1, 2020

or needs to call

 rpc.staking.unbond(addr, int(state['bonded'] - fee), enckey=enckey, name='target')

instead of

 rpc.staking.unbond(addr, int(state['bonded']), enckey=enckey, name='target')

?

(or something like state['bonded']-min_required+1 ?)

Yes, fixed now, drone exec runs successfully locally now.

@tomtau
Copy link
Contributor

tomtau commented May 1, 2020

bors r+

@yihuang yihuang requested a review from tomtau May 1, 2020 08:50
@bors
Copy link
Contributor

bors bot commented May 1, 2020

Build succeeded:

@bors bors bot merged commit 9803e3d into crypto-com:master May 1, 2020
yihuang added a commit to yihuang/chain that referenced this pull request May 1, 2020
1516: Problem (Fix crypto-com#1515): unbond tx don't subtract fee from bonded r=tomtau a=yihuang

Solution:
- Fix the bug and add unit test

Co-authored-by: yihuang <huang@crypto.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants