-
Notifications
You must be signed in to change notification settings - Fork 57
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
Update examples to use source auth and the built in token contract #128
Conversation
timelock/src/lib.rs
Outdated
|
||
verify_and_consume_nonce(&env, &from_id, &BigInt::zero(&env)); | ||
let from_id = from.identifier(&env); | ||
|
||
// Authenticate depositor with nonce of zero, so that this may |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is no longer relevant, as 0 nonce doesn't prevent anything for invoker signatures and we have a separate init check anyway.
@@ -38,34 +37,11 @@ fn create_claimable_balance_contract(e: &Env) -> ClaimableBalanceContractClient | |||
ClaimableBalanceContractClient::new(e, contract_id) | |||
} | |||
|
|||
fn sign_args( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should either cover non-source-account signatures or not support them. If we want to keep examples simple we should probably go with the latter (then we can just remove Signature
argument from the contract )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contract should support all types of auth. The tests just show the source account model since that's what we expect people to use for meridian. We can add the non-source uses of auth later once we finalize what complex auth should look like.
This is fine for this PR. Let's not block merging on it. Ideally we find another way to bundle this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. There's one issue with dependencies I see.
It looks like most of the more complex examples are continuing to use the soroban-auth
crate. I think this is good, those are already advanced examples, and the lift to use soroban-auth is small.
liquidity_pool/Cargo.toml
Outdated
|
||
[dependencies] | ||
soroban-sdk = "0.0.4" | ||
soroban-auth = "0.0.4" | ||
soroban-token-spec = "0.0.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The token spec shouldn't be a dependency. We could make it a dependency only for non-wasm builds, or for dev/tests only, but if it is included here the token contract will be reexported in the liquidity pool contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Removed references to the token spec.
#![allow(unused)] | ||
use soroban_sdk::{Bytes, BytesN, Env}; | ||
|
||
soroban_sdk::contractimport!(file = "../soroban_token_spec.wasm"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the contract spec is imported here, there should be no need to import it in the dependencies.
Resolves #122 and #77
@leighmcculloch I copied the wasm file from
soroban-token-spec
into this repo, which isn't ideal. Should the examples build the spec instead?@dmkozh I added you as a reviewer so you could take a look at the timelock contract change (e91c81b).