Skip to content
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.

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

Offline Digital Euro #157

Open
wants to merge 119 commits into
base: master
Choose a base branch
from
Open

Conversation

Robert-ML
Copy link

Offline money transfer application with double spent token detection and trust loss for the detected double spender.

AdiDumi and others added 30 commits March 8, 2023 11:08
Still not fully functional
…he Trustchain Superapp to have access to the initialized IPv8 network
…s initialized correctly compared to what I tried to use previously.

Solved hashing BUG.
The application now shows the promise as a QR code.

Possible bug: transition automatically to set amount page, to be investigated.
 into adrians

� Conflicts:
�	offlinemoney/build.gradle
�	offlinemoney/src/main/java/nl/tudelft/trustchain/offlinemoney/MainActivityOfflineMoney.kt
�	offlinemoney/src/main/java/nl/tudelft/trustchain/offlinemoney/ui/OfflineMoneyBaseFragment.kt
Copy link
Collaborator

@OrestisKan OrestisKan left a comment

Choose a reason for hiding this comment

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

Seems alright, no major issue was identified but there are some bad practices in the code that should be addressed before the code enters the production android app.

Comment on lines 30 to 31
// @Query("DELETE FROM userdata_table")
// suspend fun deleteUserData()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete unused code

Comment on lines +12 to +19
internal val privateKey: PrivateKey = defaultCryptoProvider.keyFromPrivateBin(
byteArrayOf(
76, 105, 98, 78, 97, 67, 76, 83, 75, 58, -29, -114, 126, -47, -39, -5, 22, 89,
94, 71, -1, 118, -30, 120, -8, -75, 2, 102, 99, -21, 57, -95, 124, 126, -30, 33,
-99, 37, -125, -105, 20, -45, 94, 2, -109, 125, 98, -52, 84, -54, -47, 13, 15, 75,
73, 11, -128, 5, -4, -101, 102, -1, -95, 33, -107, -77, -41, 89, 102, 44, 71, 107, 1, 107
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seams unsafe, should not be hardcoded in the main codebase

Copy link
Author

Choose a reason for hiding this comment

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

This is only for the demo in order to be able sign generated tokens on any phone. In a real scenario this would not be here and we would use only the publick key to verify the signature. If we remove this, we break the demo functionality of the app. Is it ok to leave it here or how to you sugest to manage the situation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Variable names in this file and the variable names in nl.tudelft.trustchain.offlinedigitaleuro.databinding.PrintMoneyFragmentBinding are not very explanatory, consider changing them

Comment on lines 34 to 36
// kotlinOptions {
// jvmTarget = '1.8'
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete commented out code

Comment on lines 27 to 29
// viewBinding {
// enabled = true
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete commented out code


adapter.registerRenderer(TransactionItemRenderer())

lifecycleScope.launchWhenResumed {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this launched in the background or will it lead to a chokehold?

Copy link
Author

Choose a reason for hiding this comment

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

It is launched in the background and run only once.

Comment on lines 37 to 44
lifecycleScope.launchWhenResumed {
while(isActive) {
val items = db.webOfTrustDao().getAllTrustScores()
.map { trustScore: WebOfTrust -> TrustScoreItem(trustScore) }
adapter.updateItems(items)
adapter.notifyDataSetChanged()
delay(1000L)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as the other file,is this launched in the background or may it lead to a chokehold?

Copy link
Author

Choose a reason for hiding this comment

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

It is launched in the background and run only once now after the edit.


// completes a transaction and inserts the tokens in the DB
// returns true if the operation succeeded and on failure also returns the message
@SuppressLint("SimpleDateFormat")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a good practice

// completes the transaction by deleting the transferred tokens from the DB and some other stuff
// returns true if everything went fine
// otherwise returns false and the error message
@SuppressLint("SimpleDateFormat")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a good practice

Copy link
Collaborator

Choose a reason for hiding this comment

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

delete this file

@AdiDumi AdiDumi temporarily deployed to apk_build April 26, 2023 09:58 — with GitHub Actions Inactive
@Robert-ML
Copy link
Author

Hello @OrestisKan
Can you take another look over our PR to determine if we addressed all your concerns about the best practices?

@Robert-ML Robert-ML requested a review from OrestisKan May 19, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants