Add support for mint, burn, and transfer #215

awrichar · 2021-09-29T12:54:38Z

No description provided.

codecov-commenter · 2021-09-29T12:58:29Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (8451379) to head (4156a73).
⚠️ Report is 4163 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #215      +/-   ##
===========================================
+ Coverage   99.89%   100.00%   +0.10%     
===========================================
  Files         207       214       +7     
  Lines       11335     11755     +420     
===========================================
+ Hits        11323     11755     +432     
+ Misses         11         0      -11     
+ Partials        1         0       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

awrichar · 2021-09-30T16:57:23Z

~~E2E tests currently failing due to dependency on hyperledger/firefly-tokens-erc1155#24~~

awrichar · 2021-09-30T19:45:16Z

internal/assets/tokens_transferred.go

+			if err := am.database.UpsertTokenTransfer(ctx, transfer); err != nil {
+				log.L(ctx).Errorf("Failed to record token transfer '%s': %s", transfer.ProtocolID, err)
+				return err
+			}


The logic below is relying strongly on the transactional nature of the database in order to end up with the correct account balances even in the face of potential retries. Just want to confirm that's OK.

Side note: The ERC1155 plugin actually allows balance querying, so we could include the ending balance with each transfer event instead of just the delta... but that seems more complex on the plugin side and probably won't be possible with other plugins, e.g. a UTXO model.

Well, I started typing: "Given this would need to be implemented differently for each database type, I wonder if it's worth pushing down to the database layer." ... then realized below you've done that already 👍

For NoSQL, for example you'd likely put the increment operation into an atomic instruction in the DB, rather than using a multi-query transction semantic (as we're doing with SQL, under the RunAsGroup).

peterbroadhurst

... see notes about identity mapping. Want to make sure we've got our ducks in a row on that item, before going any further with the review.

db/migrations/postgres/000030_create_tokentransfer_table.up.sql

internal/assets/manager.go

awrichar · 2021-10-01T14:31:58Z

@peterbroadhurst thanks for the initial look. I was already certain that my handling of identities was wrong, so the comments are helpful to steer my thinking a bit. I'll come up to speed on where #192 is at - agree we probably want to get that shaped up for merge before this propagates the existing problems further.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Also enable sync-async calls for token transfers (and honor the "confirm" query parameter). Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar · 2021-10-07T14:04:17Z

This is now fully reworked on top of the identity manager changes.

peterbroadhurst

... intermediate comment while I continue through the code here

peterbroadhurst · 2021-10-07T15:00:17Z

internal/assets/tokens_transferred.go

+			if err := am.database.UpsertTokenTransfer(ctx, transfer); err != nil {
+				log.L(ctx).Errorf("Failed to record token transfer '%s': %s", transfer.ProtocolID, err)
+				return err
+			}


Well, I started typing: "Given this would need to be implemented differently for each database type, I wonder if it's worth pushing down to the database layer." ... then realized below you've done that already 👍

For NoSQL, for example you'd likely put the increment operation into an atomic instruction in the DB, rather than using a multi-query transction semantic (as we're doing with SQL, under the RunAsGroup).

peterbroadhurst · 2021-10-07T15:04:15Z

internal/assets/tokens_transferred.go

+			}
+			if transfer.Type != fftypes.TokenTransferTypeMint {
+				balance.Identity = transfer.From
+				balance.Amount = -transfer.Amount


Have you considered the datatype for Amount?
e.g. is a 64bit int considered large enough, when decimals are taken into account. Or do we need a big.Int represented as a String on the JSON (and appropriate backend DB storage).

Yea, the same thing has been nagging at me. I suspect we may want Big Int. Perhaps worth a consult from @jimthematrix as well?

from the ethereum side, amount values are represented in uint256, with web3 golang SDKs using a binding with big.Int. that should be sufficient to inform our decision here.

in addition, in support of conventions in that community, we should consider supporting both decimal and hexadecimal (required to have 0x prefixes) strings for inputs.

Will allow for tying transfers to messages in a future commit. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

peterbroadhurst

Ok - worked through all the changes... and found myself raising a spelling one for you to ponder. But it's not necessary to resolve in this PR, as the Tokens work is still overall in a pre-release flux state.

peterbroadhurst · 2021-10-07T18:27:18Z

pkg/database/plugin.go

-	"tokenindex": &StringField{},
-	"identity":   &StringField{},
-	"balance":    &Int64Field{},
+	"poolprotocolid": &StringField{},


How will local_id in the database interact with the query filters?

Added a commit for this

peterbroadhurst · 2021-10-07T18:30:28Z

pkg/fftypes/operation.go

+	OpTypeTokensTransfer OpType = ffEnum("optype", "tokens_transfer")
 )


I note a difference in the OpType and EventType semantics of:

Events token_

Ops: tokens_

Wonder if that's intentional and makes sense to be different, or if we should pick one of the two here?

You're right, should probably make this consistent.

peterbroadhurst · 2021-10-07T18:36:15Z

pkg/fftypes/tokenaccount.go

-	TokenIndex string `json:"tokenIndex,omitempty"`
-	Identity   string `json:"identity,omitempty"`
-	Balance    int64  `json:"balance"`
+	PoolProtocolID string `json:"poolProtocolId,omitempty"`


Having I'm sure been part of suggesting it, I'm finding protocolId (particularly when uplifted to poolProtocolId) a bit weird in this context.
I wonder if in Tokens there's something simpler. Whether just identifier (/poolIdentifier) would make sense (as it's the thing you should use in all cases), and then maybe ffid for the local_id field (which we've discussed isn't really local in all cases).

I think Identifier would be bad since pools already have an ID field (and we need to specifically differentiate the ID assigned by FireFly from the one assigned by the connector). We do use BackendID in some cases (I think just on Operations) - not sure if that seems any better/worse that ProtocolID in this instance?

And as it's evolving, I think local_id is likely to be local-only (ie ultimately different on all nodes), so I think leaving that for the time being is correct.

jimthematrix · 2021-10-07T19:28:47Z

internal/tokens/fftokens/fftokens.go

+type mintTokens struct {
+	PoolID     string `json:"poolId"`
+	To         string `json:"to"`
+	Amount     int64  `json:"amount"`


as discussed above, should use big.Int here to accommodate possible range with Ethereum uint256

jimthematrix · 2021-10-07T19:34:25Z

internal/tokens/fftokens/fftokens.go

+	eventName := "Transfer"
+	if t == fftypes.TokenTransferTypeMint {
+		eventName = "Mint"
+	}


missing the branch for when the transfer type is Burn?

jimthematrix · 2021-10-07T19:37:23Z

internal/tokens/fftokens/fftokens.go

+	if tokenIndex == "" ||
+		poolProtocolID == "" ||
+		operatorAddress == "" ||
+		value == "" ||


I must be confused here, I thought tokenIndex is only for non-fungibles, and value (for amount) is only for fungibles, so one of them is always going to be empty?

The API agreement with the connector states that tokenIndex will always be included in the websocket events sent back to FireFly. My particular ERC1155 connector will always send tokenIndex=0 for operations on fungible pools.

Would not require a change here, but it occurs to me that I could change the input of the ERC1155 connector to work without specifying tokenIndex for transfers in fungible pools. That would remove the need for the user to specify tokenIndex in the REST call (currently the call will fail at the connector level if tokenIndex is missing).

Actually I guess I could make tokenIndex optional throughout this code as well, if it was optional to the connector.

jimthematrix · 2021-10-07T19:47:38Z

internal/tokens/fftokens/fftokens.go

+		SetBody(&mintTokens{
+			PoolID:     mint.PoolProtocolID,
+			To:         mint.To,
+			Amount:     mint.Amount,


this is not obvious for non-fungible tokens, if there's only one integer value, is that treated as the token index, or the count of tokens to be minted (with the indexes to be assigned by the contract)?

we should allow the input to specify the desired index to mint for an NFT. but not sure if that's possible with the current 1155 contract implementation

It is not possible with the current Solidity contract. It always mints the next available index in the pool.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

This is not relevant for fungible pools, so should be optional at the discretion of the token connector. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

peterbroadhurst · 2021-10-08T12:36:55Z

Raised https://github.com/hyperledger/firefly-cli/issues/109 to track E2E failure

awrichar · 2021-10-08T16:22:41Z

E2E tests can be retriggered after hyperledger/firefly-ethconnect#160 is integrated.
Coverage gap in broadcast package is being addressed by #236.

peterbroadhurst

Willing to be told I'm wrong here, but a 1024 character VARCHAR field feels wrong to me for an integer column. I understand big.Int is arbitrary precision and as such a base 10 string representation could get very long.

For me it feels like we should define a fftypes.BigInt type, and have on it Scan and Value functions that are safe in terms of the bounds that we set (like we do for UUID in https://github.com/hyperledger/firefly/blob/main/pkg/fftypes/uuid.go)

Then it also should have a deliberate choice on the JSON side (marshal/unmarshal) as to how we represent it on the JSON on output, and what formats we accept input from.

My gut says we should:

Cap it at 32byte values
DB serialize it as a Hex string (so it's fixed up to 64 bytes)
JSON serialize it as base-10 string by default
- Write the code so if we wanted to provide a different option for Hex/Number return we could do that via FireFly config
Support deserializing it from JSON per the rules here:
- https://pkg.go.dev/math/big#Int.SetString

I'd be happy to help with this, if you agree @awrichar ?

awrichar · 2021-10-08T17:22:27Z

@peterbroadhurst yep, I'm not sure where the max decimal representation for uint256 comes out, but I think it's somewhere in the 70-80 digit range. 1024 is excessive but seemed to be what we've used in many spots. Happy to have some helpers for marshaling and unmarshaling big int though if you want to take that on!

peterbroadhurst · 2021-10-08T19:25:27Z

Thanks @awrichar I'll add a commit as such to this branch, and ask @jimthematrix for a review before merging.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst · 2021-10-09T02:58:23Z

I've added in an fftypes.BigInt type with the rules I think make sense by default for backend Hex serialization, and frontend JSON serialization (any supported in, base-10 string out).

Not wired up to the other types yet.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst · 2021-10-11T12:47:35Z

I've made the changes here discussed on BigInt consistency.
e2e failing now due to tokens connector expecting a Number:

    restclient.go:65: <!! {"error":"FF10274: Error from tokens service: {\u0026#34;statusCode\u0026#34;:400,\u0026#34;message\u0026#34;:[\u0026#34;amount must not be less than 1\u0026#34;,\u0026#34;amount must be an integer number\u0026#34;],\u0026#34;error\u0026#34;:\u0026#34;Bad Request\u0026#34;}"}
    restclient.go:66: Headers: map[Content-Length:[277] Content-Type:[application/json] Date:[Mon, 11 Oct 2021 12:45:37 GMT] Vary:[Origin]]
    restclient.go:326: 
        	Error Trace:	restclient.go:326
        	            				e2e_test.go:399
        	Error:      	Not equal: 
        	            	expected: 202
        	            	actual  : 500
        	Test:       	TestE2ETokenPool
        	Messages:   	POST /namespaces/default/tokens/erc1155/pools/pool0/mint [500]: {"error":"FF10274: Error from tokens service: {\u0026#34;statusCode\u0026#34;:400,\u0026#34;message\u0026#34;:[\u0026#34;amount must not be less than 1\u0026#34;,\u0026#34;amount must be an integer number\u0026#34;],\u0026#34;error\u0026#34;:\u0026#34;Bad Request\u0026#34;}"}
    e2e_test.go:229: Websocket 127.0.0.1:5000 closing, error: read tcp 127.0.0.1:40524->127.0.0.1:5000: use of closed network connection
    e2e_test.go:229: Websocket 127.0.0.1:5001 closing, error: read tcp 127.0.0.1:34938->127.0.0.1:5001: use of closed network connection
    e2e_test.go:217: WebSockets closed

peterbroadhurst · 2021-10-11T15:53:56Z

Latest error from tokens connector after hyperledger/firefly-tokens-erc1155#32:

�[34;1mtokens_0_1        |�[0m �[32m[Nest] 17  - �[39m10/11/2021, 3:52:08 PM �[32m    LOG�[39m �[38;5;3m[RequestLogging] �[39m�[32mPOST /api/v1/init�[39m
�[34;1mtokens_0_1        |�[0m �[31m[Nest] 17  - �[39m10/11/2021, 3:52:08 PM �[31m  ERROR�[39m �[38;5;3m[ExceptionsHandler] �[39m�[31mrxjs_1.lastValueFrom is not a function�[39m
�[34;1mtokens_0_1        |�[0m TypeError: rxjs_1.lastValueFrom is not a function
�[34;1mtokens_0_1        |�[0m     at RouterResponseController.transformToResult (/root/node_modules/@nestjs/core/router/router-response-controller.js:32:27)
�[34;1mtokens_0_1        |�[0m     at /root/node_modules/@nestjs/core/router/router-execution-context.js:173:52
�[34;1mtokens_0_1        |�[0m     at /root/node_modules/@nestjs/core/router/router-execution-context.js:47:19
�[34;1mtokens_0_1        |�[0m     at processTicksAndRejections (internal/process/task_queues.js:95:5)
�[34;1mtokens_0_1        |�[0m     at async /root/node_modules/@nestjs/core/router/router-proxy.js:9:17

nguyer

Looks good besides the failing E2E. Hopefully that's not too hard to track down and fix.

awrichar · 2021-10-12T14:45:06Z

BigInt changes look great @peterbroadhurst; thanks for handling this and the piece in the tokens connector.

awrichar requested review from nguyer, nickgaski and peterbroadhurst as code owners September 29, 2021 12:54

awrichar mentioned this pull request Sep 29, 2021

Token support #218

Closed

awrichar force-pushed the transfer branch from a4a456f to a6f4e0c Compare September 30, 2021 15:57

awrichar changed the title ~~Add database types and route for listing token transfers~~ Add support for mint and transfer Sep 30, 2021

awrichar commented Sep 30, 2021

View reviewed changes

awrichar changed the title ~~Add support for mint and transfer~~ Add support for mint, burn, and transfer Sep 30, 2021

peterbroadhurst reviewed Oct 1, 2021

View reviewed changes

db/migrations/postgres/000030_create_tokentransfer_table.up.sql Outdated Show resolved Hide resolved

internal/assets/manager.go Outdated Show resolved Hide resolved

peterbroadhurst mentioned this pull request Oct 4, 2021

Add identity manager and API input matrix logic #192

Merged

awrichar added 9 commits October 7, 2021 10:01

Add database types for token transfers

1cd6c1f

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Add route for listing token transfers

acedc3f

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Add route for minting tokens

bc8c0c3

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Handle token mint events

a908f6e

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Add token minting to E2E test

90a037e

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Add LocalID to token transfers to enable event tracking

816a853

Also enable sync-async calls for token transfers (and honor the "confirm" query parameter). Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Add support for token transfers

ed7119c

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Add database events for token transfer create/update

6cafe72

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Add support for token burn

2372a97

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar force-pushed the transfer branch from 903e160 to 2372a97 Compare October 7, 2021 14:02

awrichar force-pushed the transfer branch 2 times, most recently from a58d206 to 48fa0c0 Compare October 7, 2021 18:00

peterbroadhurst reviewed Oct 7, 2021

View reviewed changes

Add message hash and tx info to token transfer

f2b4cc3

Will allow for tying transfers to messages in a future commit. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar force-pushed the transfer branch from 48fa0c0 to f2b4cc3 Compare October 7, 2021 18:37

peterbroadhurst reviewed Oct 7, 2021

View reviewed changes

jimthematrix suggested changes Oct 7, 2021

View reviewed changes

awrichar added 3 commits October 7, 2021 21:13

Use big.Int for token amounts

fb21e6e

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Clean up some token operations and queries

80fc909

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Make tokenIndex optional

2ecf521

This is not relevant for fungible pools, so should be optional at the discretion of the token connector. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Merge branch 'main' of github.com:hyperledger/firefly into HEAD

ccbf260

jimthematrix approved these changes Oct 8, 2021

View reviewed changes

peterbroadhurst reviewed Oct 8, 2021

View reviewed changes

awrichar mentioned this pull request Oct 8, 2021

Add transaction for token transfers #239

Merged

Add fftypes.BigInt foundation type

b31df50

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst added 2 commits October 11, 2021 08:34

Move to fftypes.BigInt

6be0f02

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

Add coverage for new error path

4156a73

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst mentioned this pull request Oct 11, 2021

Use strings to pass all values, as they are uint256 hyperledger/firefly-tokens-erc1155#32

Merged

nguyer approved these changes Oct 11, 2021

View reviewed changes

peterbroadhurst mentioned this pull request Oct 11, 2021

Rsjx version hyperledger/firefly-tokens-erc1155#33

Merged

peterbroadhurst merged commit da6a0fd into hyperledger:main Oct 11, 2021

peterbroadhurst deleted the transfer branch October 11, 2021 17:03

		OpTypeTokensTransfer OpType = ffEnum("optype", "tokens_transfer")
		)

Add support for mint, burn, and transfer #215

Add support for mint, burn, and transfer #215

Uh oh!

Conversation

awrichar commented Sep 29, 2021

Uh oh!

codecov-commenter commented Sep 29, 2021 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

awrichar commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awrichar Sep 30, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

peterbroadhurst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

awrichar commented Oct 1, 2021

Uh oh!

awrichar commented Oct 7, 2021

Uh oh!

peterbroadhurst 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!

peterbroadhurst 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!

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!

awrichar Oct 8, 2021 • 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!

awrichar Oct 8, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbroadhurst commented Oct 8, 2021

Uh oh!

awrichar commented Oct 8, 2021

Uh oh!

peterbroadhurst left a comment

Choose a reason for hiding this comment

Uh oh!

awrichar commented Oct 8, 2021

codecov-commenter commented Sep 29, 2021 •

edited by codecov bot

Loading

awrichar commented Sep 30, 2021 •

edited

Loading

awrichar Sep 30, 2021 •

edited

Loading

awrichar Oct 8, 2021 •

edited

Loading

awrichar Oct 8, 2021 •

edited

Loading

peterbroadhurst commented Oct 9, 2021 •

edited

Loading

nguyer left a comment •

edited

Loading