hashgraph / hedera-nft-auction-demo Goto Github PK

Describe the solution you'd like

Upon creation of an auction, it is possible to specify a list of keys with a threshold

It should be possible to specify more complex key structures, for example a list containing two threshold lists.

This needs to be implemented both at the command line level (./gradlew command) and admin rest api.

examples:

{
  "keylist": [
    {
      "keys": [
        "3021....",
	"3022.....",
	"3023...."
      ],
      "threshold": 2
    },
    {
      "keys": [
        "3024....",
	"3025....."
      ]
    }
  ]
}

which would result in a list of keys, the first a threshold 2 of 3 threshold key, the second a list of two keys

Note: there is no need to nest further than this, a list containing lists or thresholds should be sufficient.

Describe the solution you'd like

When the auction ends, the nodes should check for an association between the winning account and the token

Describe the bug
Environment variables are troublesome in JS builds, there is an API (port 8081) which returns the network in use which if possible the UI should use so no environment variables are required.

Is your feature request related to a problem? Please describe.
When the auction ends, it's not clear it's awaiting association from the winner to the auctioned token.

Describe the solution you'd like
The auction's status should make it clear association is pending from the winner, only after this will the token be transferred.

Additional context
Back-end support may be required, or just display as a message upon auction closing (status CLOSED). Once the transfer takes places, the auction ends (status ENDED).

Describe the bug
An attacker may adjust the auctionFile attribute supplied to the admin/auction endpoint to expose the contents of arbitrary files within the Docker machine or exhaust memory with large files leading to a denial of service

Expected behavior
Files should be read from a safe location only and any supplied file names should not include relative paths.

Describe the bug
Owing to scheduled transactions not supporting account updates for now, this operation is run as a standard operation using a single "master" key.

It should be using a scheduled transaction when this is possible.

Is your feature request related to a problem? Please describe.
Additional .env parameters are included in the UI environment file which could easily be obtained from the backend instead.

• VUE_APP_NETWORK=testnet
• VUE_APP_TOPIC_ID=0.0.412980
• VUE_APP_NODE_OWNER=

Describe the solution you'd like
Remove this environment variables
Provide a back end API to make this information available to the UI

Describe alternatives you've considered
Do nothing, this however creates an additional configuration step which could be avoided.

Additional context
This proposed change doesn't change any functionality, it merely makes the deployment easier.

Is your feature request related to a problem? Please describe.
There is a need to support a "root" threshold key which could contain other threshold keys, need to support a more flexible nested structure.

Describe the solution you'd like
Enable any key structure to be represented and created against the account

Association between the token and the auction account is currently done via a traditional transaction, this needs to be done via a scheduled transaction.

Need to add unit testing to the project, including testing the database integration

Expected behavior

When a new auction is notified via a HCS message, the auction nodes should create a scheduled transaction to associate with the token.

Is your feature request related to a problem? Please describe.
This api would force a restart of the applications (if running in docker-compose, it would automatically restart) so that a new topic id can be taken into account easily.

This should also delete all data in the database.

Describe the solution you'd like
Implement an admin rest API to "switch to new topic id"
Posts a message to the current topic containing new topicId
When message received, delete all data from database
Update topicId in .env
Exit the application which will restart automatically and pick up the new topic id if run in docker-compose.

Is your feature request related to a problem? Please describe.
The repo is currently missing tests, specifically for the Verticle processes.

Describe the solution you'd like
Add a suite of tests that give coverage for the API Verticles

You can utilize vertx-unit to provide the test context and easy to spin up.

Recommendation for coverage

1. Admin API - Verify expected input actions, verify error handling for erroneous and duplicate inputs
   1. postTopicHandler - "/v1/admin/topic"
   2. postCreateToken - "/v1/admin/token"
   3. postAuctionAccountHandler - "/v1/admin/auctionaccount"
   4. postTransferHandler - "/v1/admin/transfer"
   5. postAuctionHandler - "/v1/admin/auction"
   6. postEasySetupHandler - "/v1/admin/easysetup"
2. Bidder API - Verify expected input actions, verify error handling for erroneous and non existent inputs
   1. getAuctionHandler - "/v1/auctions/:id"
   2. getAuctionsHandler - "/v1/auctions"
   3. getLastBidderBidHandler - "/v1/lastbid/:auctionid/:bidderaccountid"
   4. getBidsHandler - "/v1/bids/:auctionid"

Describe alternatives you've considered
Use a different test context framework

Additional context
Add any other context or screenshots about the feature request here.

Describe the bug
In the event a node is offline for a period of time, when it next starts, it may attempt to process refunds that have already been completed by other nodes.

Expected behavior
The node should first check if a refund completed and if so, make the bid as refunded and not issue a scheduled transaction to attempt to refund.

Describe the solution you'd like

The bid dialog allows any amount to be input

The bid dialog should prevent bids below the current winning bid being input.

Describe the bug
The /v1/environment endpoint returns the following information
{ "network": "testnet", "topicId": "0.0.2011472", "nodeOperator": "Master" }

Would be good to display the "nodeOperator" (which could be blank/undefined) if available. This will indicate which of the auction validators' UI is being shown.

Would be good to display the TopicID (with a link to dragon glass) so users of the UI can witness the auction setup data against the consensus service.

Describe the bug
An overlooked Unicode normalization step may result in user confusion or even a divergence between application instances that breaks interoperability.

Expected behavior
Inputs should be normalized in all cases to avoid divergence.

Is your feature request related to a problem? Please describe.
It may be desirable to highlight particular auctions with a "featured" flag.

Describe the solution you'd like
Enable auctions to carry a featured flag so that they can be highlighted accordingly in the UI.

Describe the bug
The HTTP protocol lacks support for the encryption, authentication and integrity provided by its close sibling HTTPS, and thus allows an attacker on the network to easily observe, modify and replay network traffic contents. Given the distributed and sensitive nature of the system, insecure transport must be avoided.

Describe the bug
When an auction complete, funds remain in the auction account, they should be transferred to the account that transferred the token to the auction account (not the treasury of the token) so that this original account is "paid" for the token on auction complete.
Otherwise, the auction account remains the holder of the funds, requiring a subsequent multi-sig transaction to transfer the funds to the original token owner.

Expected behavior
As part of the token transfer to the winner, transfer the winning bid to the account that transferred the token to the auction account.

Is your feature request related to a problem? Please describe.
The auction demo utilizes many calls to the network to either create entities, transfer crypto or tokens and create scheduled transactions.
However, we're missing tests to prevent regression

Describe the solution you'd like
Add tests classes for each client

1. CreateToken
2. CreateTopic
3. CreateTokenTransfer
4. CreateAuctionAccount

Describe alternatives you've considered

Additional context
Note that most methods are static.

Is your feature request related to a problem? Please describe.
A number of abstract classes and interfaces were introduced in preparation for supporting different mirror nodes' REST apis.
Calls to mirror nodes are now implemented in a single re-usable method meaning this can later be enhanced to support other apis without the need to mirror-specific implementations of data consumers as is the case now.

Describe the solution you'd like
Remove the abstract classes and interfaces to improve code readability.

Describe alternatives you've considered
Do nothing.

Is your feature request related to a problem? Please describe.
For easy spin up It would be good to

Describe the solution you'd like
The README would greatly benefit from an initial value proposition and illustration of an expected auctions operations , highlighting each person and each module.

Then in a separate section each modules operation should be summarized e.g. auction node, observer node, REST API, auction UI

Describe alternatives you've considered

Additional context
This initial part doesn't have to have in depth architecture. This can be broken out later

Describe the solution you'd like
Only Hedera is supported, should support additional mirrors such as Dragonglass

Describe the solution you'd like

Any bid amount increase is allowed in the bid dialog

If the auction has a minimum bid increase amount, the dialog should ensure the bid is at least current winning bid + minimum amount

There currently is no incentive model in place for running a validator node, we could consider paying each node participating in validation a share of a fee on each bid, or on the winning bid.

e.g. 1% fee on bids, equally shared amongst participating nodes (maybe the share is weighted against actual participation such as signatures on scheduled transactions)

or, 1% fee on winning bid, sharing per the above.

If implemented, this should be optional and the fees configurable per auction or network.

Describe the solution you'd like
All refund transactions are currently "normal" transactions, need to switch to scheduled transactions

Describe the bug
Readonly nodes don't have a key to sign scheduled transactions and therefore don't update their database with the necessary information.

Expected behavior
Even without the key to sign a scheduled transaction, readonly nodes can generate the deterministic scheduled transaction id and hash, update their database and watch for completion of the scheduled transaction like "validator" nodes.
The only difference is that they don't submit the scheduled transaction.

Describe the bug
The UI shows hbar as a unit for reserve and minimum bid increase, however the values given by the REST API are expressed in tinybars (100,000 tiny bar in a hbar).

Expected behavior
The UI should represent the minimum bid and reserve in the correct tiny bar unit.

Is your feature request related to a problem? Please describe.
It is desirable to show a list of validators in the UI such that seeing who is in the list of validators and navigating to their own UI instance is possible.

Describe the solution you'd like

• Add database objects to store a list of validators (url, name, optionally public key)
• Add an API to add/remove a validator from the database
• When the API is called, submit the add/remove request to HCS
• When subscribing to HCS, handle the add/remove requests accordingly
• Enhance the /v1/environment API endpoint to include the list of validators from the database

Describe the bug
Insufficient input validation on APIs may expose undesired or undetermined behaviour.
Develop an expected schema for all API endpoint input, and ensure all received parameters are validated as early as possible and as strictly as possible.

Expected behavior
APIs should validate input data as much as possible

• Duplicate or extraneous JSON fields should result in full rejection.
• Integers should have a minimum and maximum magnitude.
• Strings should have a minimum and maximum length. In many cases, the character set can be constrained to prevent vulnerabilities such as directory traversal. In many cases, Unicode normalization should be applied.
• Enumerated types should be explicitly checked for exactly one match.
• Objects having a specialized format, such as account IDs and URLs, should have their format validated.

Is your feature request related to a problem? Please describe.
The UI design calls for a title and description for each auction, this is currently missing

Describe the solution you'd like
Either implement #9 or at a minimum a title and description for each auction.

Describe the bug
the code currently issues a scheduled transaction for a number of operations, then polls a mirror node to check for the success of the transaction. If the transaction failed for some reason (e.g. not enough signatures in time), the auction would keep polling forever.

Need to introduce an additional check such that in the event of a failed scheduled transaction, it is re-issued to avoid being stuck in a loop.

Is your feature request related to a problem? Please describe.
The repo could benefit from easy to run E2E test that cover supported auction scenarios.
The test suite should support runs against a real env

Describe the solution you'd like

• Deploy docker images for Db, and auction node with available configurations. Auction node should support easy setup along with environment variable or config file edits
• Add SDK client that will support transactions submission for accounts, tokens, topics and schedule transactions by an operator
• Add configuration file to capture host details of the db, adminAPI and bidder REST API
• Add test suite with context startup utilizing the TestContainer features to spin up the db and auction node containers
• Add a beforeAll() or similar that starts a new auction and calls to the adminAPI to getAuctions() to get the auction details
• Based on configurations an appropriate number of new crypto accounts should be created to represent the bidders
• Test methods should contain logic to send appropriate bids through crypto transfer submissions to the network. Test methods should also make calls to REST API to confirm auction bid details and status

Describe alternatives you've considered
A single container could be used per test class, in that case each method should create a new auction using the adminAPI

Additional context
Suggestion E2E scenarios can be

• SimpleAuctionClass
  • BeforeAll method calls AdminAPI get getAuctions and picks first
  • Using Auction details to pull tokenId, topicId, auction account
  • Class uses or create 2 or 3 funded crypto bidder accounts
  • Method will play out logic of bids.
  • Bidder and Admin REST API calls can be used for verification of actions
• EdgeAuctionClass
  • Out of time
  • Long running randomized auction bid values
  • High number of bidders

Is your feature request related to a problem? Please describe.
When creating a topic id for the auction system, it's necessary to restart it in order for the new topic id to be taken into account.

Describe the solution you'd like
Creating a new topic should automatically be picked up by the topic subscriber

Describe the bug
When an auction starts, if there are no bidders, the token owner should get their token back when the auction ends.

Describe the bug
When clicking on a bid, we're taken to dragon glass, the link is hardcoded to testnet, should be using environment variables to detect testnet or mainnet.

Describe the solution you'd like

When the auction ends, and the token is associated with the winning account, the auction nodes should optionally transfer the token to the winning account by way of a scheduled transactions.

This should be an optional feature which is determined by a boolean in a .env variable

Describe the solution you'd like

When the auction completes (last timestamp > auction end timestamp), set the auction account to have receive signature enabled to prevent further bids being send to the auction account.

This should be done via a scheduled transaction

Is your feature request related to a problem? Please describe.
Currently each call to HederaClient.getClient() creates a new SDK client and doesn't close it's connections
Additionally the "clients" that interact with the network are classes that offer static methods to run single operations.
There may be improvements that could be made to allow for easy referencing and potential performance gains

Describe the solution you'd like

• Move clients to a single directory each focusing on the entity or transaction type
• Have a single SDK client created at process startup that is utilized in general. If additional are needed due to high TPS e.g. a dedicated client for transfers
• Remove file system logic from TokenClient. Let create() instead take extracted String contents. This should make it easier to test
• Close clients at end of auction

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context

Describe the solution you'd like
Only Hedera is supported, should support additional mirrors such as Kabuto

Describe the bug
Owing to scheduled transactions not supporting token association for now, this operation is run as a standard operation using a single "master" key.

It should be using a scheduled transaction when this is possible.

Describe the bug
When creating an auction account, a master key has to be supplied. This is currently part of the JSON payload sent to the app. This could be removed from the JSON payload and the app could add the master key to the keylist automatically if present in the .env file.

Expected behavior
When creating a new auction account, if a master key is present in .env, add it to the threshold key for the account along with the other keys such that:

threshold key (1) Master Key + Threshold key (n) auction account keys

Describe the bug
The bid history is limited to the last 50 bids, this should be added to the "History" label if possible.

• Might be nice to see auctions in some sort of table view that have ended, but not necessarily sold

An example of an "Ended (not sold)" auction would be an auction that was live, but did not receive any bids throughout the entirety of its auction life. This auction would end without being sold, as there would be no winning bid.

Is your feature request related to a problem? Please describe.
The REST api is missing some useful endpoints

Describe the solution you'd like
Add the following endpoints
-/v1/reservenotmetauctions : auctions where the winning bid < reserve, any auction status
-v1/closedauctions : auctions which are closed, meaning a winner has been determined, any future bids will be refunded regardless of amount
-/v1/endedauctions : auctions which are closed, the winner received the token and the original token owner received the payment in exchange (note that an auction which has transferToWinner=false will jump from closed to ended immediately since there is no no token transfer to take place.
-/v1/activeauctions : auctions which are able to receive bids (may include reserve not met auctions)
-/v1/pendingauctions : auctions that are awaiting transfer of the token being auctioned to the auction account

Describe alternatives you've considered
Query all auctions and figure out their status from the data

Integration testing with Hedera network is desirable.

Is your feature request related to a problem? Please describe.
Creating tokens doesn't allow for a memo to be setup, it should be possible.

Describe the solution you'd like
Specify an optional memo when creating a token

Describe alternatives you've considered
None

Additional context
A memo could be useful in the context of an NFT to store the hash of some associated data, or a link to the location where this data is stored.

Describe the bug
An adversary could repeat its public key, threshold times, during auction account creation and control the auction.

Expected behavior
Duplicate keys should result in an error

Describe the solution you'd like

The token may currently be associated with an image (stored as a Hedera file), it would be desirable to following this Hedera Improvement Proposal for the token's metadata.

https://github.com/hashgraph/hedera-improvement-proposal/issues/40