feat(nano): Add address balance in global state#1431
Conversation
|
| Branch | feat/nano-address-balance |
| Testbed | ubuntu-22.04 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result minutes (m) (Result Δ%) | Lower Boundary minutes (m) (Limit %) | Upper Boundary minutes (m) (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | 📈 view plot 🚷 view threshold | 1.66 m(-2.73%)Baseline: 1.71 m | 1.54 m (92.53%) | 2.05 m (81.06%) |
8899037 to
c63ed89
Compare
c63ed89 to
67993eb
Compare
67993eb to
d99ce6c
Compare
eb416fa to
21d7909
Compare
d99ce6c to
572f68d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1431 +/- ##
==========================================
- Coverage 85.68% 85.66% -0.02%
==========================================
Files 442 444 +2
Lines 33846 34155 +309
Branches 5293 5347 +54
==========================================
+ Hits 29000 29260 +260
- Misses 3824 3859 +35
- Partials 1022 1036 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # XXX Should we fail? | ||
| return | ||
|
|
||
| # XXX Should we check for the size to prevent miscalling with a contract id? |
There was a problem hiding this comment.
Yes! Transfer to contracts should be made exclusively by deposits.
| raise NanoContractDoesNotExist(contract_id.hex()) | ||
| token_proxy = TokenProxy(self) | ||
| return NCContractStorage(trie=trie, nc_id=contract_id, token_proxy=token_proxy) | ||
| block_proxy = TokenProxy(self) |
There was a problem hiding this comment.
Maybe rename TokenProxy to RestrictedBlockProxy?
| @@ -51,3 +51,6 @@ def create_token( | |||
| token_symbol=token_symbol, | |||
| token_version=token_version | |||
| ) | |||
|
|
|||
| def add_address_balance(self, address: Address, amount: Amount, token_id: TokenUid) -> None: | |||
| return Amount(balance) | ||
|
|
||
| def add_address_balance(self, address: Address, amount: Amount, token_id: TokenUid) -> None: | ||
| key = AddressBalanceKey(address, token_id) |
There was a problem hiding this comment.
Shouldn't we assert that address is an actual address (never a contract id)?
| fees=fees or (), | ||
| ) | ||
|
|
||
| def transfer_to_address(self, address: Address, amount: Amount) -> None: |
There was a problem hiding this comment.
What about the token id?
| raise TooManySigOps(f'sigops count greater than max: {sigops_count} > {MAX_SCRIPT_SIGOPS_COUNT}') | ||
|
|
||
| try: | ||
| raw_script_eval( |
There was a problem hiding this comment.
Make sure it works properly for P2SH.
| tx2.nc_transfer_input = 10 HTR main | ||
| tx2.nc_transfer_output = 10 HTR main |
There was a problem hiding this comment.
Should we allow an input and an output of the same token? I guess not.
There was a problem hiding this comment.
I just checked the verifier and it is not allowed. How's this test passing? Are they using different addresses?
There was a problem hiding this comment.
Missing test adding to the mempool with an output greater than the available balance.
There was a problem hiding this comment.
Missing test where the execution with transfer headers is successful. Actually, at least three tests, one with transfer headers, another with a contract transfer, and, finally, another with transfer headers and contract transfer.
There was a problem hiding this comment.
Missing test where the execution fails (but not because of the transfer). In this case, the transfer should not be persisted. Another test where the execution fails because of the transfer (low balance). In this case, the transfer should be persisted as well.
Co-authored-by: Jan Segre <jan@hathor.network>
572f68d to
7f00f0f
Compare
Motivation
What was the motivation for the changes in this PR?
Acceptance Criteria
Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged