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

Extended signing support for Off-chain message signing #8

Open
wants to merge 3 commits into
base: compute-budget-rf-v2
Choose a base branch
from

Conversation

0xMMBD
Copy link
Member

@0xMMBD 0xMMBD commented Dec 14, 2023

  • Off-chain message signing implemented according to: https://docs.solana.com/proposals/off-chain-message-signing
  • Modified general item screens to fix memory issues on nanos
  • Empty application domain will be displayed as "Domain not provided" instead of base58 representation
  • Messages Includes changes for ComputeBudget instruction

0xMMBD added a commit that referenced this pull request Dec 14, 2023
Copy link

@jnwng jnwng left a comment

Choose a reason for hiding this comment

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

still looks like this is missing Trent's original commit, so a bit hard to see what the adjustments are. but a couple of things to address

libsol/Makefile Outdated
@@ -18,7 +18,6 @@ ifeq ($(COVERAGE),1)
CFLAGS += --coverage
endif

debug_CFLAGS = -g -fsanitize=address -fsanitize=undefined
Copy link

Choose a reason for hiding this comment

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

why are these changes needed again?

@@ -83,8 +83,12 @@
0x00, 0x00

// Sysvars

Copy link

Choose a reason for hiding this comment

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

remove whitespace change

* 4. Message format (1 byte)
* 5. Signer count (1 bytes)
* 6. Signers (signer_count * 32 bytes) - assume that only one signer is present
* 7. Message length (2 bytes)
Copy link

Choose a reason for hiding this comment

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

can you link to the specification?

@@ -59,16 +59,28 @@ typedef struct MessageHeader {
size_t instructions_length;
} MessageHeader;

//@TODO move to offchain message sign .h
Copy link

Choose a reason for hiding this comment

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

address TODO

#include <stdbool.h>

bool is_data_utf8(const uint8_t *data, size_t length);
bool is_data_ascii(const uint8_t *data, size_t length);
Copy link

Choose a reason for hiding this comment

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

add EOL

};

typedef enum SummaryItemKind SummaryItemKind_t;

typedef struct SummaryItem SummaryItem;

extern char G_transaction_summary_title[TITLE_SIZE];

// Text buffer needs to be large enough to hold the longest possible message text to sign
//#define TEXT_BUFFER_LENGTH (OFFCHAIN_MESSAGE_MAXIMUM_MESSAGE_LENGTH - OFFCHAIN_MESSAGE_MINIMAL_HEADER_SIZE)
Copy link

Choose a reason for hiding this comment

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

why is this commented out?

libsol/parser.c Outdated
Comment on lines 155 to 163
* Field Start offset Length (bytes)
* Signing Domain 0x00 16
* Header version 0x10 1
* Application domain 0x11 32
* Message format 0x31 1
* Signer count 0x32 1
* Signers 0x33 SIGNER_COUNT * 32
* Message length 0x33 + SIGNER_CNT * 32 2
*/
Copy link

Choose a reason for hiding this comment

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

can you get these lined up?

Copy link
Member Author

Choose a reason for hiding this comment

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

What tool are you using for code review? On my end it is formatted quite well (github and CLion), so it's hard to fix because I'm not really able to reproduce it.
Maybe we can just replace it with the link to the docs? This way we don't have to worry about formatting

image

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed it, I hope that it helps on you end, if not we can remove it and leave just the link

@@ -148,6 +198,19 @@ int transaction_summary_set_fee_payer_pubkey(const Pubkey* pubkey) {
return 0;
}

/*
* Supress warning about const qualifier - changes required in ledger's SDK
Copy link

Choose a reason for hiding this comment

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

address typo

* fixed comments formatting
* removed unnecessary whitespace changes
@0xMMBD
Copy link
Member Author

0xMMBD commented Jan 5, 2024

@jnwng Let us know if there is anything else to fix 😄

@0xMMBD 0xMMBD requested a review from jnwng February 19, 2024 08:48
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.

2 participants