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

Add SignMessageObject to signMessage function #32

Closed
wants to merge 3 commits into from

Conversation

DOBEN
Copy link
Member

@DOBEN DOBEN commented Mar 31, 2023

Purpose

The browser wallet can now sign bytes as well. Expanding the library to enable passing an object (with schema) to be signed.

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

@@ -20,7 +20,7 @@
"build": "tsc"
},
"dependencies": {
"@concordium/browser-wallet-api-helpers": "^2.0.0",
"@concordium/browser-wallet-api-helpers": "^2.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this upgrade necessitates changes to signAndSendTransaction:

Argument of type 'Record<string, unknown>' is not assignable to parameter of type 'SmartContractParameters'.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DOBEN I've found that doing this part properly is a bigger change that I've done separately. Please see #34.

@DOBEN DOBEN force-pushed the signing-browser-wallet-permit-object branch from ef54158 to fee8b5a Compare April 3, 2023 14:13
@@ -69,7 +70,7 @@ export interface WalletConnection {
accountAddress: string,
type: AccountTransactionType.Update | AccountTransactionType.InitContract,
payload: SendTransactionPayload,
parameters: Record<string, unknown>,
parameters: SmartContractParameters,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add this interface change to the changelog. Is it a breaking change @shjortConcordium ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The "new" types are what the parameters can actually be, so while this change is breaking, it is only breaking for inputs that wouldn't work, so it is more a bugfix than an actual breaking change 🤔

@@ -204,7 +205,7 @@ export class WalletConnectConnection implements WalletConnection {
accountAddress: string,
type: AccountTransactionType,
payload: AccountTransactionPayload,
parameters?: Record<string, unknown>,
parameters?: SmartContractParameters,
Copy link
Contributor

@bisgardo bisgardo Apr 4, 2023

Choose a reason for hiding this comment

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

Does the mobile wallet support the new types? @shjortConcordium If not, I think we should only include the parts of the union type that are supported everywhere in the interface. I assume the browser wallet implementation could extend this to the full type.

Copy link
Contributor

Choose a reason for hiding this comment

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

The mobile should "support" these types, but the parameters are serialized before sending them to the MW.

@bisgardo
Copy link
Contributor

bisgardo commented Apr 5, 2023

I can't get the call with a primitive string type to work with the mobile wallet: The parameter serialization fails as boiled down to the test

const { Buffer } = require('buffer/');
const { serializeUpdateContractParameters } = require('@concordium/common-sdk');

test('serialize plain string parameter', () => {
    const contractName = 'test';
    const receiveName = 'string';
    const parameters = 'xyz';
    const schema = 'FgI';
    const schemaVersion = 2;
    const res = serializeUpdateContractParameters(
        contractName,
        receiveName,
        parameters,
        Buffer.from(schema, 'base64'),
        schemaVersion
    );
    console.log({res});
});

which fails with the error

unable to deserialize parameters, due to: Parse error

@concordium/common-sdk is at version 6.4.1 which is the current version.

To run the test, add the test file to the project root, install jest (yarn add --dev jest) and run jest.

The test is based on the following example contract written by @DOBEN (deployed on testnet with index 4239):

use concordium_std::*;
#[derive(Serialize)]
struct State;
/// Init function that creates a new contract.
#[init(contract = "test")]
fn init<S: HasStateApi>(
    _ctx: &impl HasInitContext,
    _state_builder: &mut StateBuilder<S>,
) -> InitResult<()> {
    Ok(())
}
pub type Number = u64;
#[receive(
    contract = "test",
    name = "number",
    parameter = "Number",
)]
fn number<S: HasStateApi>(
    ctx: &impl HasReceiveContext,
    _host: &impl HasHost<State, StateApiType = S>,
) -> ReceiveResult<()> {
    let _ = ctx.parameter_cursor().get()?;
    Ok(())
}
pub type Bool = bool;
#[receive(
    contract = "test",
    name = "bool",
    parameter = "Bool",
)]
fn bool<S: HasStateApi>(
    ctx: &impl HasReceiveContext,
    _host: &impl HasHost<State, StateApiType = S>,
) -> ReceiveResult<()> {
    let _ = ctx.parameter_cursor().get()?;
    Ok(())
}
pub type St = String;
#[receive(
    contract = "test",
    name = "string",
    parameter = "St",
)]
fn string<S: HasStateApi>(
    ctx: &impl HasReceiveContext,
    _host: &impl HasHost<State, StateApiType = S>,
) -> ReceiveResult<()> {
    let _ = ctx.parameter_cursor().get()?;
    Ok(())
}

which produces the schema

{
  "contractName": "test",
  "entrypoints": {
    "bool": {
      "parameter": "AQ"
    },
    "number": {
      "parameter": "BQ"
    },
    "string": {
      "parameter": "FgI"
    }
  }
}

@shjortConcordium @abizjak is there something wrong with the way I call serializeUpdateContractParameters? What should be done to make the test pass?

@shjortConcordium
Copy link
Contributor

shjortConcordium commented Apr 5, 2023

If you want to use serializeUpdateContractParameters you need to supply the schema for the entire module. (Not the schema for the parameter itself, like "FgI" is)
Otherwise you can use serializeTypeValue(parameters, Buffer.from(schema, 'base64')) .

@bisgardo
Copy link
Contributor

bisgardo commented Apr 5, 2023

If you want to use serializeUpdateContractParameters you need to supply the schema for the entire module. (Not the schema for the parameter itself, like "FgI" is) Otherwise you can use serializeTypeValue(parameters, Buffer.from(schema, 'base64')) .

How does the implementation tell which kind of schema is supplied? What does the browser wallet do? Try both and see which one works?

@shjortConcordium
Copy link
Contributor

shjortConcordium commented Apr 5, 2023

If you want to use serializeUpdateContractParameters you need to supply the schema for the entire module. (Not the schema for the parameter itself, like "FgI" is) Otherwise you can use serializeTypeValue(parameters, Buffer.from(schema, 'base64')) .

How does the implementation tell which kind of schema is supplied? What does the browser wallet do? Try both and see which one works?

For sendTransaction, it now takes a SchemaWithContext, which specifies which type. (If given only a string as the schema it assumes the old format, for backwards compatibility)
In the SignMessageObject, it should always be the type specific schema.

@@ -122,7 +122,7 @@ export class BrowserWalletConnector implements WalletConnector, WalletConnection
return this.client.sendTransaction(accountAddress, type, payload);
}

async signMessage(accountAddress: string, message: string): Promise<AccountTransactionSignature> {
async signMessage(accountAddress: string, message: string | SignMessageObject): Promise<AccountTransactionSignature> {
Copy link
Contributor

@bisgardo bisgardo Apr 11, 2023

Choose a reason for hiding this comment

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

I don't understand this SignMessageObject type:

type SignMessageObject = {
    /** as base64 */
    schema: string;
    /** as hex */
    data: string;
};

What is schema? What is data? Why is one base64 and the other hex? @shjortConcordium

Copy link
Contributor

Choose a reason for hiding this comment

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

The schema is the schema that should be used to deserialize the message data, and we usually encode schemas with base64.
The data is the message data, as hex because we usually encode stuff other than schemas as hex 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same kind of schema used to serialize parameters? Used for the wallet to display what you sign decoded to a string? What if you want to sign arbitrary bytes that don't look like a parameter?

Is there a reason for data not being a Buffer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'll allow arbitrary bytes that cannot be displayed using a schema.

The BW API does not include buffers, seeing that they are not a "native" type in a web environment.

Copy link
Contributor

@shjortConcordium shjortConcordium left a comment

Choose a reason for hiding this comment

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

I don't think the Mobile wallets support the signMessageObject API yet, so maybe we need to have some handling for that, otherwise we are relying on the current MW versions to gracefully handle the unexpected format 🤔
(Otherwise LGTM)

@bisgardo
Copy link
Contributor

bisgardo commented May 3, 2023

@DOBEN I'm suggesting a different approach in #35 which is consistent with #34, which was just merged.

@DOBEN
Copy link
Member Author

DOBEN commented May 19, 2023

Closing this MR:

Changes that were implemented in
#34
#35
were tested in and satisfy the requirements so that this MR is obsolete.

@DOBEN DOBEN closed this May 19, 2023
@bisgardo bisgardo deleted the signing-browser-wallet-permit-object branch May 21, 2023 19:27
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

3 participants